Opened 4 years ago
Closed 4 years ago
#17618 closed enhancement (fixed)
Update matplotlib so that plot_directive is less broken
Reported by:  tmonteil  Owned by:  

Priority:  major  Milestone:  sage6.8 
Component:  packages: standard  Keywords:  
Cc:  fbissey, strogdon, kcrisman, gagern, novoselt  Merged in:  
Authors:  Thierry Monteil, François Bissey  Reviewers:  Steven Trogdon 
Report Upstream:  N/A  Work issues:  
Branch:  9dc580d (Commits)  Commit:  9dc580db712380bbcfe20c958d5125696ceb5e70 
Dependencies:  Stopgaps: 
Description (last modified by )
In #17498, some problems appeared with the plot_directive module
from matplotlib, in particular, the impossibility to remove the (Source code)
link (that points nowhere) above images.
With 1.4.2, the plot_include_source
option seems still broken but a new plot_html_show_source_link
seems to work, so let us upgrade.
Tarball: http://www.lmona.de/files/sage/matplotlib1.4.3.tar.bz2
Attachments (6)
Change History (85)
comment:1 Changed 4 years ago by
 Cc fbissey added
comment:2 Changed 4 years ago by
 Branch set to u/tmonteil/update_matplotlib_so_that_plot_directive_is_less_broken
comment:3 followup: ↓ 5 Changed 4 years ago by
 Commit set to 374eca4c6161a7216b48ebb5ea772d62c5695f11
comment:4 Changed 4 years ago by
Is the tarball ready ?
Nathann
comment:5 in reply to: ↑ 3 ; followup: ↓ 6 Changed 4 years ago by
Replying to fbissey:
I am expecting broken doctests with matplotlib 1.4.x? Did you find any?
I am indeed getting some MatplotlibDeprecationWarning
Replying to ncohen:
Is the tarball ready ?
As explained on this thread, i would like to be sure to make a reproducible tarball. If you want to test for #17498, you can use the upstream tarball meanwhile (we only remove some dirs it to save space but there is no difference otherwise).
comment:6 in reply to: ↑ 5 Changed 4 years ago by
As explained on this thread, i would like to be sure to make a reproducible tarball.
It seems to be the thread on which I mentionned a workaround using "diff R". I saw your answer but I did not understand it, and so I wait for the answer to the private email I sent you after that.
Nathann
comment:7 Changed 4 years ago by
Here are the 13 doctest failures that appeared in a make ptestlong
, quite a few are redundant:
File "src/doc/en/bordeaux_2008/introduction.rst", line 75, in doc.en.bordeaux_2008.introduction Failed example: list_plot3d(v, interpolation_type='nn') Expected: Graphics3d Object Got: doctest:137: MatplotlibDeprecationWarning: The matplotlib.delaunay module was deprecated in version 1.4. Use matplotlib.tri.Triangulation instead. Graphics3d Object
File "src/sage/graphs/generic_graph.py", line 15636, in sage.graphs.generic_graph.GenericGraph.plot Failed example: D.plot(edge_labels=True, color_by_label={'a':'blue', 'b':'red'}, edge_style='dashed') Expected: Graphics object consisting of 34 graphics primitives Got: doctest:239: FormatterWarning: Exception in text/plain formatter: 'module' object has no attribute '_Base' None
File "src/sage/plot/arrow.py", line 349, in sage.plot.arrow.Arrow._render_on_subplot Failed example: a.save(filename=filename) Exception raised: Traceback (most recent call last): File "/opt/sagemath/sage6.3/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 488, in _run self.compile_and_execute(example, compiler, test.globs) File "/opt/sagemath/sage6.3/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 850, in compile_and_execute exec(compiled, globs) File "<doctest sage.plot.arrow.Arrow._render_on_subplot[11]>", line 1, in <module> a.save(filename=filename) File "/opt/sagemath/sage6.3/local/lib/python2.7/sitepackages/sage/misc/decorators.py", line 471, in wrapper return func(*args, **kwds) File "/opt/sagemath/sage6.3/local/lib/python2.7/sitepackages/sage/plot/graphics.py", line 3048, in save figure = self.matplotlib(**options) File "/opt/sagemath/sage6.3/local/lib/python2.7/sitepackages/sage/plot/graphics.py", line 2494, in matplotlib g._render_on_subplot(subplot) File "/opt/sagemath/sage6.3/local/lib/python2.7/sitepackages/sage/plot/arrow.py", line 414, in _render_on_subplot class ConditionalStroke(pe._Base): AttributeError: 'module' object has no attribute '_Base'
File "src/sage/plot/arrow.py", line 523, in sage.plot.arrow.arrow2d Failed example: arrow2d((1, 1), (3, 3), linestyle='dashed') Expected: Graphics object consisting of 1 graphics primitive Got: doctest:239: FormatterWarning: Exception in text/plain formatter: 'module' object has no attribute '_Base' None
File "src/sage/plot/arrow.py", line 525, in sage.plot.arrow.arrow2d Failed example: arrow2d((1, 1), (3, 3), linestyle='') Expected: Graphics object consisting of 1 graphics primitive Got: None
File "src/sage/plot/colors.py", line 22, in sage.plot.colors Failed example: sorted(colormaps) Expected: ['Accent', 'Accent_r', 'Blues', 'Blues_r', 'BrBG', 'BrBG_r', ...] Got: [u'Accent', u'Accent_r', u'Blues', u'Blues_r', u'BrBG',
File "src/sage/plot/colors.py", line 1337, in sage.plot.colors.get_cmap Failed example: sorted(colormaps) Expected: ['Accent', 'Accent_r', 'Blues', 'Blues_r', ...] Got: [u'Accent', u'Accent_r', u'Blues', u'Blues_r', u'BrBG', u'BrBG_r',
File "src/sage/plot/colors.py", line 1398, in sage.plot.colors.Colormaps Failed example: sorted(colormaps) Expected: ['Accent', 'Accent_r', 'Blues', 'Blues_r', ...] Got: [u'Accent', u'Accent_r', u'Blues', u'Blues_r',
File "src/sage/plot/colors.py", line 1645, in sage.plot.colors.Colormaps.__delitem__ Failed example: maps.popitem() Expected: ('Spectral', <matplotlib.colors.LinearSegmentedColormap object at ...>) Got: (u'Spectral', <matplotlib.colors.LinearSegmentedColormap object at 0x4afd4a0c>)
File "src/sage/plot/graphics.py", line 1091, in sage.plot.graphics.Graphics.add_primitive Failed example: G Expected: Graphics object consisting of 2 graphics primitives Got: doctest:239: FormatterWarning: Exception in text/plain formatter: 'module' object has no attribute '_Base' None
File "src/sage/plot/graphics.py", line 2096, in sage.plot.graphics.Graphics.? Failed example: p._matplotlib_tick_formatter(subplot, **d) Expected: (<matplotlib.axes.AxesSubplot object at ...>, <matplotlib.ticker.MaxNLocator object at ...>, <matplotlib.ticker.MaxNLocator object at ...>, <matplotlib.ticker.OldScalarFormatter object at ...>, <matplotlib.ticker.OldScalarFormatter object at ...>) Got: (<matplotlib.axes._subplots.AxesSubplot object at 0x4b8da48c>, <matplotlib.ticker.MaxNLocator object at 0x4bbe3dac>, <matplotlib.ticker.MaxNLocator object at 0x4bbe3f2c>, <matplotlib.ticker.OldScalarFormatter object at 0x4bbe388c>, <matplotlib.ticker.OldScalarFormatter object at 0x4bbe332c>)
File "src/sage/plot/plot3d/list_plot3d.py", line 90, in sage.plot.plot3d.list_plot3d.list_plot3d Failed example: list_plot3d(m, texture='yellow', interpolation_type='nn',frame_aspect_ratio=[1,1,1/3]) Expected: Graphics3d Object Got: doctest:137: MatplotlibDeprecationWarning: The matplotlib.delaunay module was deprecated in version 1.4. Use matplotlib.tri.Triangulation instead. Graphics3d Object
File "src/sage/stats/distributions/discrete_gaussian_lattice.py", line 133, in sage.stats.distributions.discrete_gaussian_lattice.Disc reteGaussianDistributionLatticeSampler Failed example: list_plot3d(l, point_list=True, interploation='nn') Expected: Graphics3d Object Got: doctest:137: MatplotlibDeprecationWarning: The matplotlib.delaunay module was deprecated in version 1.4. Use matplotlib.tri.Triang ulation instead. Graphics3d Object
comment:8 Changed 4 years ago by
Some redundancy indeed. Let's see what's to be done:
 Add the missing "u" to doctest (for unicode if I am not mistaken). That's one of the easiest bit.
 Get rid of calls to _base in sage/plot/*
 Migrate from delaunay to tri.Triang
 Fix sage/plot/graphics.py  easy.
 Why no output in sage/plot/arrow.py?
comment:9 followup: ↓ 12 Changed 4 years ago by
It looks like we may be able to get away with replacing delaunay with tri.Triangulation without any other changes. Another option is to switch to scipy's delaunay.
comment:10 Changed 4 years ago by
 Cc strogdon added
comment:11 Changed 4 years ago by
 Commit changed from 374eca4c6161a7216b48ebb5ea772d62c5695f11 to 4c6f6e1b58d270077dc6eb6e0fdff5b85904ceab
comment:12 in reply to: ↑ 9 Changed 4 years ago by
 Cc kcrisman added
Replying to fbissey:
It looks like we may be able to get away with replacing delaunay with tri.Triangulation without any other changes. Another option is to switch to scipy's delaunay.
A dropin replacement does not work since they do not offer the same methods. Also, we rely on nearest neighbor interpolation which, according to this page seems to require installing some natgrid package, see also this page about removing natgrid.
comment:13 followup: ↓ 14 Changed 4 years ago by
I'm not invested in this, but just be sure to check actual pictures for a good variety of plots...
comment:14 in reply to: ↑ 13 Changed 4 years ago by
Replying to kcrisman:
I'm not invested in this,
Uh, sorry for ccing you, git blame
denounced you.
but just be sure to check actual pictures for a good variety of plots...
Yes, we will look at how failed doctests are visually rendered before and after the fixes.
comment:15 Changed 4 years ago by
I, on the other hand, am very interested in this because updating matplotlib is a prerequisite for updating numpy to 1.9.x (MPL 1.3.x won't build with numpy 1.9.x).
So if we cannot use matplotlib tri.Triangulation could we try scipy's delaunay instead?
comment:16 Changed 4 years ago by
OK scipy doesn't offer nearest neighbour either. I think it is time to hit the list to know if there is any reason we should stick to nn. linear or cubic should make good default. I would go cubic myself as the default.
comment:17 Changed 4 years ago by
If no one comment on my post on sagedevel by tomorrow we just go ahead and change the default. Is this ok with you? We may need to check the documentation.
comment:18 Changed 4 years ago by
OK in the absence of comments I think we should go ahead with something.
comment:19 Changed 4 years ago by
I see you didn't put up a tarball or updated the checksums so I will do it. I am testing my current changes, I am doing a build from scratch right now.
comment:20 Changed 4 years ago by
 Branch changed from u/tmonteil/update_matplotlib_so_that_plot_directive_is_less_broken to u/fbissey/MPL1.4
 Commit changed from 4c6f6e1b58d270077dc6eb6e0fdff5b85904ceab to 8ee674d1b6e6bc2e22d7bc9f0ad9af05cce766d5
 Description modified (diff)
After changing to triangulation there are differences with what we have before but I don't know at this stage if it is better or worse. See commit.
New commits:
8ee674d  Add checksum and migrate to tri.Trinagulation

comment:21 Changed 4 years ago by
There is also a section of code that was specifically to correct data that could segfault the delaunay code. Unfortunately there is no doctest so we cannot check whether we still need it or not.
comment:22 Changed 4 years ago by
OK for the first doctest both delaunay and triangulation are bad compared to spline... Difficult to know which one is worst but nearest neighbour wasn't involved in either.
comment:23 Changed 4 years ago by
I'm coming late to the game. Aside from the actual triangulation it looks like there is still
interpolation_type='nn'
in listplot_3d.py
which gives failures for me. And in discrete_gaussian_lattice.py
there is
interploation='nn'
I guess interploation
should be interpolation
and the 'nn'
should be something else.
comment:24 Changed 4 years ago by
Actually with interpolation_type='nn'
no 3Dgraphic is generated at all. And changing 'nn'
to 'cubic'
I get:
sage t src/sage/plot/plot3d/list_plot3d.py ********************************************************************** File "src/sage/plot/plot3d/list_plot3d.py", line 138, in sage.plot.plot3d.list_plot3d.list_plot3d Failed example: list_plot3d(l,interpolation_type='cubic',texture='yellow',num_points=100) Expected: Graphics3d Object Got: doctest:3847: UserWarning: Warning: converting a masked element to nan. Graphics3d Object **********************************************************************
comment:25 Changed 4 years ago by
Yes, still work in progress, I'll have to get to all these next. We'll have to explore that last warning too.
comment:26 Changed 4 years ago by
Hi, sorry i was out of connection those days, which is why i did not answered earlier. @fbissey i am not sure 2 days is a sufficient delay to let people answer in such community as Sage, involving so much people, none of which working full time on it.
I am not convinced yet that cubic interpolation is a suitable replacement for nearest neighborhood, both in terms of speed, meaning and visual rendering, so i guess this should be inspected further with that respect (as suggested by @kcrismann in his previous answer). I will try to work on it later in the day.
I see two things:
 the
delaunay
module of matplotlib is only deprecated for now, according to the second thread i mentioned earlier: "This will be OK for a year or two, but eventually we will completely remove matplotlib.delaunay", so for now it is possible to catch the deprecation warning and get time to think further. If matplotlib's upgrade is urgent because it blocks other important packages, let us better go for it than silently forget a feature.  If matplotlib decides to get rid of its
delaunay
module, why not incorporate it in Sage's source code, this is free software after all. We also have some similar stuff in Sage that could be used as a replacement as @fbissey suggested with scipy. This can of course be the purpose of another ticket.
comment:27 Changed 4 years ago by
Things happen! Anyway if you want to have an answer to something that has been posted more than 2448hours you may need to repost again. Admitly the list has been relatively quite the past week so it may not be drawn in the flow has it would normally be.
While it is only deprecated it is only pushing the problem away in my opinion  it is a form of procrastination.
I guess we don't completely loss delaunay if we want to hold to it, we can migrate to scipy, what we will lose fast, is the nearest neighbour default. scipy doesn't have it, MPL doesn't really want it, why should we have it? Are there any technical arguments for it? Also the underlying code under delaunay is changed in any case to be based on qhull, there may be difference, even in the linear if we decide to postpone.
comment:28 Changed 4 years ago by
comment:29 Changed 4 years ago by
I may be missing something, and it may be a terminology thing, but it appears that scipy0.15 does have nearest neighbor interpolation:
https://github.com/scipy/scipy/blob/master/scipy/interpolate/interpolate.py and https://github.com/scipy/scipy/blob/master/scipy/interpolate/interpolate_wrapper.py
search for nearest
.
comment:30 Changed 4 years ago by
Indeed, I thought I read that scipy didn't have it. May be delaunay doesn't have it.
Changed 4 years ago by
Changed 4 years ago by
Changed 4 years ago by
Changed 4 years ago by
Changed 4 years ago by
comment:31 Changed 4 years ago by
I did the following test to compare the new cubic
interpolation type with the former nn
:
Creation of a common test (taken from discrete_gaussian_lattice.py
doctest):
from sage.stats.distributions.discrete_gaussian_lattice import DiscreteGaussianDistributionLatticeSampler D = DiscreteGaussianDistributionLatticeSampler(identity_matrix(2), 3.0) S = [D() for _ in range(2^12)] l = [vector(v.list() + [S.count(v)]) for v in set(S)] save(l, '/tmp/vectors.sobj')
Then, on the former 6.4:
sage: l = load('/tmp/vectors.sobj') sage: %timeit P = list_plot3d(l, interpolation_type='linear', texture="automatic", point_list=True) 100 loops, best of 3: 12.4 ms per loop
sage: %timeit P = list_plot3d(l, interpolation_type='nn', texture="automatic", point_list=True) 10 loops, best of 3: 29.8 ms per loop
On the patched 6.5.beta5:
sage: l = load('/tmp/vectors.sobj') sage: %timeit P = list_plot3d(l, interpolation_type='linear', texture="automatic", point_list=True) 100 loops, best of 3: 18.8 ms per loop
sage: %timeit P = list_plot3d(l, interpolation_type='cubic', texture="automatic", point_list=True) 1 loops, best of 3: 498 ms per loop
The generation of the cubic
interpolation is much (16x) slower that the nn
interpolation, and the visual renderings are very different (nn
looks more like a smoothed linear
while cubic
shows more spots at lattice points). So, i agree to add the cubic
interpolation type, but disagree to let this replace nn
in existing tests (too different) nor as becoming the default (too slow).
comment:32 Changed 4 years ago by
In list_plot3d.py
, the following is more than dubious, especially if drop_list
is not empty:
x=[x[i] for i in range(len(x)) if i not in drop_list] y=[y[i] for i in range(len(x)) if i not in drop_list] z=[z[i] for i in range(len(x)) if i not in drop_list]
comment:33 Changed 4 years ago by
 Branch changed from u/fbissey/MPL1.4 to u/tmonteil/MPL1.4
comment:34 followup: ↓ 35 Changed 4 years ago by
 Commit changed from 8ee674d1b6e6bc2e22d7bc9f0ad9af05cce766d5 to 065dc779e98d32ec5b081680db5356df98f15ff2
Hadn't seen your plots (not shown in email) that's cool work. I don't think the stuff with drop_list is spurious. The point is to eliminate double entries. Indices are added to drop_list if the corresponding point is identical to a previous one. The code you quote builds a new set of x, y and z where each set of point is unique.
On the other code that code in the generation of drop_list is a bit gauche
if z[i] != z[j]: ..... elif z[i] == z[j]: drop_list.append(j)
Like there was a third case? the elif could be replaced by else in my opinion.
New commits:
1b4e74e  #17618 : fix spacing (trivial)

065dc77  #17618 : fix comment 32

comment:35 in reply to: ↑ 34 ; followup: ↓ 36 Changed 4 years ago by
Replying to fbissey:
I don't think the stuff with drop_list is spurious. The point is to eliminate double entries. Indices are added to drop_list if the corresponding point is identical to a previous one. The code you quote builds a new set of x, y and z where each set of point is unique.
The problem is that, if drop_list
is not empty, then at the first line, the list x
is shortened (duplicates are removed), but then only a prefix of the lists y
and z
is dealt with (since len(x)
decreased at the first line).
On the other code that code in the generation of drop_list is a bit gauche
if z[i] != z[j]: ..... elif z[i] == z[j]: drop_list.append(j)Like there was a third case? the elif could be replaced by else in my opinion.
Indeed, it would save a useless computation to just write else
.
comment:36 in reply to: ↑ 35 Changed 4 years ago by
Replying to tmonteil:
Replying to fbissey:
I don't think the stuff with drop_list is spurious. The point is to eliminate double entries. Indices are added to drop_list if the corresponding point is identical to a previous one. The code you quote builds a new set of x, y and z where each set of point is unique.
The problem is that, if
drop_list
is not empty, then at the first line, the listx
is shortened (duplicates are removed), but then only a prefix of the listsy
andz
is dealt with (sincelen(x)
decreased at the first line).
Good spotting I understand what you mean now. The quick fix is to use len(y) and len(z) or a preset auxiliary variable as you have done some other places.
comment:37 followup: ↓ 38 Changed 4 years ago by
OK I spent some time over the scipy documentation and I think I will try to use scipy delaunay and interpolation instead. It looks doable. There may still be some differences in rendering because the back end for delaunay is different. It may take me a couple of days before getting there.
comment:38 in reply to: ↑ 37 Changed 4 years ago by
Replying to fbissey:
OK I spent some time over the scipy documentation and I think I will try to use scipy delaunay and interpolation instead. It looks doable. There may still be some differences in rendering because the back end for delaunay is different. It may take me a couple of days before getting there.
Great ! I will finish some cleanup of list_plot3d.py
meanwhile.
comment:39 followup: ↓ 40 Changed 4 years ago by
I tried to post a comment on Tuesday but trac wouldn't let me :( so in scipy we can produce a Delaunay object and use it for linear interpolation but the nearest neighbor method cannot use a Delaunay object just a list of points. The linear interpolation can use the same kind of list of points. So I think I will give the delaunay object the hard shoulder and go directly to the list of points for both.
comment:40 in reply to: ↑ 39 Changed 4 years ago by
Replying to fbissey:
I tried to post a comment on Tuesday but trac wouldn't let me :( so in scipy we can produce a Delaunay object and use it for linear interpolation but the nearest neighbor method cannot use a Delaunay object just a list of points. The linear interpolation can use the same kind of list of points. So I think I will give the delaunay object the hard shoulder and go directly to the list of points for both.
I definitely agree with that: using Delaunay triangulations eases finding nearest neighbors, but nearest neighbors can be found without that, see also this comment. The only loss we could have not using an underlying Delaunay triangulation should be with respect to speed (though computing Delaunay triangulation also costs, even with qhull
). Do you plan to use scipy.interpolate.NearestNDInterpolator
(or scipy.interpolate.griddata
) for nn
interpolation ? If yes, it seems to rely on scipy.spatial.cKDTree
which seems to be a method unrelated to relying on a Delaunay triangulation, perhaps is it faster ?
Also, it could be nice to keep both matplotlib's and scipy's linear
and cubic
interpolations in order to compare them (both visually and in terms of speed), at least for now. Depending on that, we could give the whole job to scipy
.
comment:41 Changed 4 years ago by
Delaunay would be especially useful for speed if we were reusing it. But that's not the case. I was planning on using scipy.interpolate.NearestNDInterpolator. I will try to put everything at first so we can test as you suggest. So there will be MPLlinear and SPlinear I will investigate using griddata too now that you suggested it.
I am caught in things but hopefully I can get a decent shot at it today.
comment:42 Changed 4 years ago by
OK so scipy and MPL linear interpolation look pretty much the same on screen but scipy's nn is ugly and all blocky. scipy's linear is the fastest
sage: %timeit P = list_plot3d(l, interpolation_type='linear', texture="automatic", point_list=True) 100 loops, best of 3: 6.6 ms per loop sage: %timeit P = list_plot3d(l, interpolation_type='MPLlinear', texture="automatic", point_list=True) 100 loops, best of 3: 7.71 ms per loop sage: %timeit P = list_plot3d(l, interpolation_type='nn', texture="automatic", point_list=True) 100 loops, best of 3: 4.15 ms per loop
comment:43 Changed 4 years ago by
I am trying to get griddata in before posting a new branch.
comment:44 Changed 4 years ago by
I don't think I will do griddata. griddata will output an array value for for all the x and y want at once. ParametricSurface? takes a function and a list of point to evaluate your function at. The best way to plot a known list of given [x,y,z] such as generated by griddata is listplot3d. So we would have listplot3d calling listplot3d and I have no idea on how to avoid that.
comment:45 Changed 4 years ago by
 Branch changed from u/tmonteil/MPL1.4 to u/fbissey/trac17618MPL1.4
 Commit changed from 065dc779e98d32ec5b081680db5356df98f15ff2 to b2c0045d3619b225008984e7631c3abcf4f382e8
Here is the stuff I got so far. Sorry for the delay  I am still thinking about the grid stuff. We may be able to do it but we'll have to use lower level drawing directives than what is used for the other plots.
New commits:
b2c0045  more experiments in list_plot3d

comment:46 Changed 4 years ago by
MPL 1.4.3 is out and is the latest release before 2.0, Now that sage 6.5 is officially out we may want to drive that one in if we can.
comment:47 Changed 4 years ago by
 Cc gagern added
comment:48 Changed 4 years ago by
This patch has grown a lot of "hair" (i.e. stuff unrelated to the purpose of the ticket). To what extent are all the changes in src/sage/plot/plot3d/list_plot3d.py
really needed?
comment:49 Changed 4 years ago by
A lot of the stuff is for testing and deciding which bits to include. I haven't really had feedback or suggestions for the last two months. Of course a first step will be to update the tarball to 1.4.3.
The problem seems to be that we cannot quite reproduce the previous stuff in aspect or speed with functions of MPL or scipy that are not deprecated.
comment:50 Changed 4 years ago by
 Cc novoselt added
comment:51 Changed 4 years ago by
Is anything happening with this ticket? The broken 'source code' links still appear on our online doc.
Nathann
comment:52 Changed 4 years ago by
Not lately. I or someone has to update the spkg to 1.4.3 which is the final 1.4 release of MPL before 2.0. The main problem is the deprecation of Delaunay
, we don't have anything to replace it that match the speed or the appearance in terms of plots. We can have very good slow plotting or very fast crappy plotting but nothing quite like Delaunay and that puts people a bit off.
comment:53 Changed 4 years ago by
test, please ignore
comment:54 Changed 4 years ago by
OK we need to move this forward. I suggest and will try to do in the next few days:
 update tarball to 1.4.3
 use Thierry latest branch in terms of patches
 Reintroduce Delaunay and use it as a default.
 fix any doctest warnings due to that decision
 correct the documentation. "nn" has never been "nearest neighbour" in the context of Delaunay, it means "natural neighbour" which is why the scipy "nn" interpolator looks different.
We'll probably need to deal with Delaunay in MPL 2.0 but there may be suitable replacement in that version.
comment:55 Changed 4 years ago by
 Branch changed from u/fbissey/trac17618MPL1.4 to u/fbissey/MPL1.4.3
 Commit changed from b2c0045d3619b225008984e7631c3abcf4f382e8 to b16e00b38cf554bcd55650d95a76ea17ffb4cf1b
 Description modified (diff)
The current linked tarball is suitable for testing at this stage. I am preparing a slimmed down tarball as per instructions in SPKG.txt
and will put it up when ready.
New commits:
5c9091d  Merge branch 'u/tmonteil/MPL1.4' of git://trac.sagemath.org/sage into develop

b16e00b  Update matplotlib to 1.4.3 replace some Delaunay calls by "tri". Keep Delaunay interpolations for the default "nn" (natural

comment:56 Changed 4 years ago by
 Commit changed from b16e00b38cf554bcd55650d95a76ea17ffb4cf1b to a62644f4d11251157ce9883521d15993a3bae895
Branch pushed to git repo; I updated commit sha1. New commits:
a62644f  Slim the tarball by removing images. Also commit correct version.

comment:57 Changed 4 years ago by
 Description modified (diff)
 Milestone changed from sage6.5 to sage6.8
 Status changed from new to needs_review
Ready for review.
New commits:
a62644f  Slim the tarball by removing images. Also commit correct version.

comment:58 Changed 4 years ago by
 Branch changed from u/fbissey/MPL1.4.3 to u/tmonteil/MPL1.4.3
comment:59 Changed 4 years ago by
 Commit changed from a62644f4d11251157ce9883521d15993a3bae895 to 4612049e6faa2157c09a8f453fb37ab2b993b2f6
I added a spkgsrc
file to automate the tarball's trimming.
New commits:
4612049  #17618 add a spkgsrc file to automate tarball trimming.

comment:60 Changed 4 years ago by
 Status changed from needs_review to needs_info
This looks good to me and I think in the interest of moving forward with matplotlib that the approach used is the best. I too was confused with the use of 'nn'
. I'm now running doctests to make sure everything is OK. Then I'll give positive review
. One clarifying question. Should the following typo be fixed here?
 a/src/sage/stats/distributions/discrete_gaussian_lattice.py +++ b/src/sage/stats/distributions/discrete_gaussian_lattice.py @@ 132,7 +132,7 @@ class DiscreteGaussianDistributionLatticeSampler(SageObject): sage: D = DiscreteGaussianDistributionLatticeSampler(identity_matrix(2), 3.0) sage: S = [D() for _ in range(2^12)] sage: l = [vector(v.list() + [S.count(v)]) for v in set(S)]  sage: list_plot3d(l, point_list=True, interploation='nn') + sage: list_plot3d(l, point_list=True, interpolation='nn') Graphics3d Object REFERENCES:
Nothing on this ticket caused it but it is a typo and it appeared while searching for 'nn'
.
comment:61 Changed 4 years ago by
Now that you have posted it, yes we should and I believe it is appropriate to do it in this ticket. Thierry, would you add it on top of your last commit?
comment:62 Changed 4 years ago by
 Commit changed from 4612049e6faa2157c09a8f453fb37ab2b993b2f6 to 30fb859c8e77cc76adb35f6fe8ef44a36215d35d
Branch pushed to git repo; I updated commit sha1. New commits:
30fb859  #17618 fix typo in discrete_gaussian_lattice.py

comment:63 Changed 4 years ago by
 Status changed from needs_info to needs_review
Cool, back to need_review.
comment:64 Changed 4 years ago by
 Reviewers set to Steven Trogdon
 Status changed from needs_review to positive_review
This works for me.
comment:65 followup: ↓ 66 Changed 4 years ago by
+[ n "${SAGE_ROOT}" ]  SAGE_ROOT="$(pwd)/../../../"
Isn't it cleaner to just stop (as spkginstall
does when SAGE_LOCAL
is not defined)?
comment:66 in reply to: ↑ 65 Changed 4 years ago by
 Status changed from positive_review to needs_info
Replying to ncohen:
+[ n "${SAGE_ROOT}" ]  SAGE_ROOT="$(pwd)/../../../"Isn't it cleaner to just stop (as
spkginstall
does whenSAGE_LOCAL
is not defined)?
Perhaps something does need to be done. I don't know what the protocol for these spkgsrc
scripts is. The present script does work from a sage shell. However, if not in a sage shell, it fails with something like
tar (child): /64bitdev/storage/sagegit_develop/sage/../../..//upstream/matplotlib1.4.3.tar.bz2: Cannot open: No such file or directory tar (child): Error is not recoverable: exiting now
unless executed from the build/pkgs/matplotlib
folder. So setting this back to needs_info
until it can be resolved.
comment:67 Changed 4 years ago by
 Commit changed from 30fb859c8e77cc76adb35f6fe8ef44a36215d35d to 32afbe15955286dfaf2f3f270ca2e231aa8f0af7
Branch pushed to git repo; I updated commit sha1. New commits:
32afbe1  #17618 : add some information on how to use spkgsrc.

comment:68 followup: ↓ 70 Changed 4 years ago by
 Status changed from needs_info to needs_review
Replying to ncohen:
+[ n "${SAGE_ROOT}" ]  SAGE_ROOT="$(pwd)/../../../"Isn't it cleaner to just stop (as
spkginstall
does whenSAGE_LOCAL
is not defined)?
The checks done to some spkginstall
scripts are here to ensure that the script is run from the Sage shell, because they use its features (e.g. some environment variables like ${MAKE}
). Here, in contrary to such spkginstall
scripts, there is no need to check that since the script do not use such feature, except ${SAGE_ROOT}
that can be recovered from the current location.
Replying to strogdon:
Perhaps something does need to be done. I don't know what the protocol for these
spkgsrc
scripts is. The present script does work from a sage shell. However, if not in a sage shell, it fails with something liketar (child): /64bitdev/storage/sagegit_develop/sage/../../..//upstream/matplotlib1.4.3.tar.bz2: Cannot open: No such file or directory tar (child): Error is not recoverable: exiting nowunless executed from the
build/pkgs/matplotlib
folder. So setting this back toneeds_info
until it can be resolved.
The whole "protocol" can be found here, which is pretty vague regarding the importance to be run from the Sage shell.
How i see it, this script is intented to be run by maintainers, ease of use is prefered over security, which is pretty different from spkginstall
that is run by users and should be pretty strict regarding misuses. The aim is not to have a script that could be run from anywhere, obfuscated with tons of tests, but should deserve the following purposes:
 document and make transparent the differences between the upstream tarball and the one we distribute (hence the comments above each paragraph),
 be a handy tool to save some time for next updates (hence the autodiscovery of latest version and download url, and the possibility to be run from the developer's shell),
 i would add perfect reproductibility of distributed tarballs, but it seems that nobody cares.
comment:69 followup: ↓ 71 Changed 4 years ago by
What about replacing "#!/usr/bin/env bash" with "#!/usr/bin/env sage sh" ? Wouldn'it solve the problem in an elegant way? We would not have to check for those environment variables anymore, and that would only require 'sage' can be found in PATH
(which is easier than requiring SAGE_ROOT to be set)? Errors would be reported by bash itself (i.e. 'sage' not found) when needed?
comment:70 in reply to: ↑ 68 Changed 4 years ago by
 i would add perfect reproductibility of distributed tarballs, but it seems that nobody cares.
That's wrong (the 'nobody cares' part, of course :P
)
comment:71 in reply to: ↑ 69 ; followup: ↓ 72 Changed 4 years ago by
Replying to ncohen:
What about replacing "#!/usr/bin/env bash" with "#!/usr/bin/env sage sh" ?
I think that if you put a command with spaces after "#!/usr/bin/env", the behavior is either unpredictable or just wrong (= not what you expect). I forget which. In any case, I think it's a bad idea.
comment:72 in reply to: ↑ 71 ; followup: ↓ 74 Changed 4 years ago by
I think that if you put a command with spaces after "#!/usr/bin/env", the behavior is either unpredictable or just wrong (= not what you expect). I forget which. In any case, I think it's a bad idea.
Works for me. Can you illustrate your claim with examples, possibly online bug reports? It seems that "env  sage sh" does what it should.
comment:73 Changed 4 years ago by
Oh. You are right, it totally breaks when it's the first line of a bash script.
comment:74 in reply to: ↑ 72 Changed 4 years ago by
Replying to ncohen:
I think that if you put a command with spaces after "#!/usr/bin/env", the behavior is either unpredictable or just wrong (= not what you expect). I forget which. In any case, I think it's a bad idea.
Works for me. Can you illustrate your claim with examples, possibly online bug reports? It seems that "env  sage sh" does what it should.
For reference, see http://mailindex.netbsd.org/netbsdusers/2008/11/09/msg002388.html.
comment:75 followup: ↓ 77 Changed 4 years ago by
I don't think checking for SAGE_ROOT
is necessarily a bad idea. And the protocol doesn't seem to require the script to be run from the Sage shell. What if something like the following, in addition to the latest commit, were added
 a/build/pkgs/matplotlib/spkgsrc +++ b/build/pkgs/matplotlib/spkgsrc @@ 2,6 +2,10 @@ set e +printf "This script should be run as ./spkgsrc from the repository to which it +belongs, or from the Sage shell.\n" +read rsp $'Press any key to continue or \'CTRL+C\' to exit :\n' n1 key + [ n "${SAGE_ROOT}" ]  SAGE_ROOT="$(pwd)/../../../"
Off hand I don't know how to check if one is in
the correct repository. And perhaps giving the warning is enough.
comment:76 Changed 4 years ago by
 Commit changed from 32afbe15955286dfaf2f3f270ca2e231aa8f0af7 to 9dc580db712380bbcfe20c958d5125696ceb5e70
Branch pushed to git repo; I updated commit sha1. New commits:
9dc580d  #17618 spkgsrc can be run from anywhere.

comment:77 in reply to: ↑ 75 ; followup: ↓ 78 Changed 4 years ago by
Replying to strogdon:
I am not favorable to developing interactive user interfaces for spkgsrc
scripts.
comment:78 in reply to: ↑ 77 Changed 4 years ago by
 Status changed from needs_review to positive_review
comment:79 Changed 4 years ago by
 Branch changed from u/tmonteil/MPL1.4.3 to 9dc580db712380bbcfe20c958d5125696ceb5e70
 Resolution set to fixed
 Status changed from positive_review to closed
I am expecting broken doctests with matplotlib 1.4.x? Did you find any?
New commits:
#17618 package version
#17618 remove upstreamapplied patches
#17618 update setup.py.patch
#17618 update SPKG.txt