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: sage-6.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 fbissey)

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/matplotlib-1.4.3.tar.bz2

Attachments (6)

vectors.sobj (4.4 KB) - added by tmonteil 4 years ago.
6.4_linear.png (65.0 KB) - added by tmonteil 4 years ago.
6.4_nn.png (72.9 KB) - added by tmonteil 4 years ago.
6.5_linear.png (62.1 KB) - added by tmonteil 4 years ago.
6.5_cubic.png (71.8 KB) - added by tmonteil 4 years ago.
SP_nn.jpg (30.5 KB) - added by fbissey 4 years ago.
scipy nearest neighbor - sage 6.5.beta5

Download all attachments as: .zip

Change History (85)

comment:1 Changed 4 years ago by fbissey

  • Cc fbissey added

comment:2 Changed 4 years ago by tmonteil

  • Branch set to u/tmonteil/update_matplotlib_so_that_plot_directive_is_less_broken

comment:3 follow-up: Changed 4 years ago by fbissey

  • Commit set to 374eca4c6161a7216b48ebb5ea772d62c5695f11

I am expecting broken doctests with matplotlib 1.4.x? Did you find any?


New commits:

2fdfecf#17618 package version
ef72f4b#17618 remove upstream-applied patches
188c737#17618 update setup.py.patch
374eca4#17618 update SPKG.txt

comment:4 Changed 4 years ago by ncohen

Is the tarball ready ?

Nathann

comment:5 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by tmonteil

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 ncohen

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 tmonteil

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/sage-6.3/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/opt/sagemath/sage-6.3/local/lib/python2.7/site-packages/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/sage-6.3/local/lib/python2.7/site-packages/sage/misc/decorators.py", line 471, in wrapper
        return func(*args, **kwds)
      File "/opt/sagemath/sage-6.3/local/lib/python2.7/site-packages/sage/plot/graphics.py", line 3048, in save
        figure = self.matplotlib(**options)
      File "/opt/sagemath/sage-6.3/local/lib/python2.7/site-packages/sage/plot/graphics.py", line 2494, in matplotlib
        g._render_on_subplot(subplot)
      File "/opt/sagemath/sage-6.3/local/lib/python2.7/site-packages/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 fbissey

Some redundancy indeed. Let's see what's to be done:

  1. Add the missing "u" to doctest (for unicode if I am not mistaken). That's one of the easiest bit.
  2. Get rid of calls to _base in sage/plot/*
  3. Migrate from delaunay to tri.Triang
  4. Fix sage/plot/graphics.py - easy.
  5. Why no output in sage/plot/arrow.py?

comment:9 follow-up: Changed 4 years ago by 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.

comment:10 Changed 4 years ago by strogdon

  • Cc strogdon added

comment:11 Changed 4 years ago by git

  • Commit changed from 374eca4c6161a7216b48ebb5ea772d62c5695f11 to 4c6f6e1b58d270077dc6eb6e0fdff5b85904ceab

Branch pushed to git repo; I updated commit sha1. New commits:

bf4b81b#17618 fix bugs involving a missing _Base class in sage/plot/arrow.py
12baa7f#17618 fix unicode doctest failures in colors.py
4c6f6e1#17618 fix _subplots doctest failure in plot/graphics.py

comment:12 in reply to: ↑ 9 Changed 4 years ago by tmonteil

  • Authors set to Thierry Monteil
  • 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 drop-in 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 follow-up: Changed 4 years ago by kcrisman

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 tmonteil

Replying to kcrisman:

I'm not invested in this,

Uh, sorry for cc-ing 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 fbissey

I, on the other hand, am very interested in this because updating matplotlib is a pre-requisite 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 fbissey

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 fbissey

If no one comment on my post on sage-devel 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 fbissey

OK in the absence of comments I think we should go ahead with something.

comment:19 Changed 4 years ago by fbissey

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 fbissey

  • Branch changed from u/tmonteil/update_matplotlib_so_that_plot_directive_is_less_broken to u/fbissey/MPL-1.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:

8ee674dAdd checksum and migrate to tri.Trinagulation

comment:21 Changed 4 years ago by fbissey

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 fbissey

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 strogdon

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 strogdon

Actually with interpolation_type='nn' no 3D-graphic 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 fbissey

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 tmonteil

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 fbissey

Things happen! Anyway if you want to have an answer to something that has been posted more than 24-48hours you may need to re-post 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 jpflori

Slightly related, I've opened #17642, #17643, #17644 for numpy/scipy/sympy updates (currently, nothing to see there though).

comment:29 Changed 4 years ago by strogdon

I may be missing something, and it may be a terminology thing, but it appears that scipy-0.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 fbissey

Indeed, I thought I read that scipy didn't have it. May be delaunay doesn't have it.

Changed 4 years ago by tmonteil

Changed 4 years ago by tmonteil

Changed 4 years ago by tmonteil

Changed 4 years ago by tmonteil

Changed 4 years ago by tmonteil

comment:31 Changed 4 years ago by tmonteil

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 tmonteil

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 tmonteil

  • Branch changed from u/fbissey/MPL-1.4 to u/tmonteil/MPL-1.4

comment:34 follow-up: Changed 4 years ago by fbissey

  • 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 ; follow-up: Changed 4 years ago by 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 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 fbissey

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 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).

Good spotting I understand what you mean now. The quick fix is to use len(y) and len(z) or a pre-set auxiliary variable as you have done some other places.

comment:37 follow-up: Changed 4 years ago by 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.

comment:38 in reply to: ↑ 37 Changed 4 years ago by tmonteil

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 follow-up: Changed 4 years ago by 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.

comment:40 in reply to: ↑ 39 Changed 4 years ago by tmonteil

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 fbissey

Delaunay would be especially useful for speed if we were re-using 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 fbissey

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

The first one is scipy's linear. nn is fast but blockyscipy nearest neighbor - sage 6.5.beta5

Changed 4 years ago by fbissey

scipy nearest neighbor - sage 6.5.beta5

comment:43 Changed 4 years ago by fbissey

I am trying to get griddata in before posting a new branch.

comment:44 Changed 4 years ago by fbissey

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 fbissey

  • Branch changed from u/tmonteil/MPL-1.4 to u/fbissey/trac17618-MPL-1.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:

b2c0045more experiments in list_plot3d

comment:46 Changed 4 years ago by fbissey

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 gagern

  • Cc gagern added

comment:48 Changed 4 years ago by jdemeyer

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 fbissey

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 novoselt

  • Cc novoselt added

comment:51 Changed 4 years ago by ncohen

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 fbissey

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 dimpase

test, please ignore

comment:54 Changed 4 years ago by fbissey

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
  • Re-introduce 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 fbissey

  • Authors changed from Thierry Monteil to Thierry Monteil, François Bissey
  • Branch changed from u/fbissey/trac17618-MPL-1.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:

5c9091dMerge branch 'u/tmonteil/MPL-1.4' of git://trac.sagemath.org/sage into develop
b16e00bUpdate 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 git

  • Commit changed from b16e00b38cf554bcd55650d95a76ea17ffb4cf1b to a62644f4d11251157ce9883521d15993a3bae895

Branch pushed to git repo; I updated commit sha1. New commits:

a62644fSlim the tarball by removing images. Also commit correct version.

comment:57 Changed 4 years ago by fbissey

  • Description modified (diff)
  • Milestone changed from sage-6.5 to sage-6.8
  • Status changed from new to needs_review

Ready for review.


New commits:

a62644fSlim the tarball by removing images. Also commit correct version.

comment:58 Changed 4 years ago by tmonteil

  • Branch changed from u/fbissey/MPL1.4.3 to u/tmonteil/MPL1.4.3

comment:59 Changed 4 years ago by tmonteil

  • Commit changed from a62644f4d11251157ce9883521d15993a3bae895 to 4612049e6faa2157c09a8f453fb37ab2b993b2f6

I added a spkg-src file to automate the tarball's trimming.


New commits:

4612049#17618 add a spkg-src file to automate tarball trimming.

comment:60 Changed 4 years ago by strogdon

  • 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 fbissey

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 git

  • 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 fbissey

  • Status changed from needs_info to needs_review

Cool, back to need_review.

comment:64 Changed 4 years ago by strogdon

  • Reviewers set to Steven Trogdon
  • Status changed from needs_review to positive_review

This works for me.

comment:65 follow-up: Changed 4 years ago by ncohen

+[ -n "${SAGE_ROOT}" ] || SAGE_ROOT="$(pwd)/../../../"

Isn't it cleaner to just stop (as spkg-install does when SAGE_LOCAL is not defined)?

comment:66 in reply to: ↑ 65 Changed 4 years ago by strogdon

  • 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 spkg-install does when SAGE_LOCAL is not defined)?

Perhaps something does need to be done. I don't know what the protocol for these spkg-src 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/sage-git_develop/sage/../../..//upstream/matplotlib-1.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.

Last edited 4 years ago by strogdon (previous) (diff)

comment:67 Changed 4 years ago by git

  • 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 spkg-src.

comment:68 follow-up: Changed 4 years ago by tmonteil

  • 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 spkg-install does when SAGE_LOCAL is not defined)?

The checks done to some spkg-install 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 spkg-install 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 spkg-src 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/sage-git_develop/sage/../../..//upstream/matplotlib-1.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.

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 spkg-install 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 follow-up: Changed 4 years ago by ncohen

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 ncohen

  • i would add perfect reproductibility of distributed tarballs, but it seems that nobody cares.

That's wrong (the 'nobody cares' part, of course :-P)

Last edited 4 years ago by ncohen (previous) (diff)

comment:71 in reply to: ↑ 69 ; follow-up: Changed 4 years ago by jhpalmieri

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 ; follow-up: Changed 4 years ago by 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.

comment:73 Changed 4 years ago by ncohen

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 jhpalmieri

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://mail-index.netbsd.org/netbsd-users/2008/11/09/msg002388.html.

comment:75 follow-up: Changed 4 years ago by strogdon

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/spkg-src
+++ b/build/pkgs/matplotlib/spkg-src
@@ -2,6 +2,10 @@
 
 set -e
 
+printf "This script should be run as ./spkg-src 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 git

  • Commit changed from 32afbe15955286dfaf2f3f270ca2e231aa8f0af7 to 9dc580db712380bbcfe20c958d5125696ceb5e70

Branch pushed to git repo; I updated commit sha1. New commits:

9dc580d#17618 spkg-src can be run from anywhere.

comment:77 in reply to: ↑ 75 ; follow-up: Changed 4 years ago by tmonteil

Replying to strogdon:

I am not favorable to developing interactive user interfaces for spkg-src scripts.

comment:78 in reply to: ↑ 77 Changed 4 years ago by strogdon

  • Status changed from needs_review to positive_review

Replying to tmonteil:

Replying to strogdon:

I am not favorable to developing interactive user interfaces for spkg-src scripts.

It's not really the best. I'm good with this now. Let's hope it sticks.

comment:79 Changed 4 years ago by vbraun

  • Branch changed from u/tmonteil/MPL1.4.3 to 9dc580db712380bbcfe20c958d5125696ceb5e70
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.