search

sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.21k stars 421 forks source link

Update matplotlib so that plot_directive is less broken #17618

Closed edd8e884-f507-429a-b577-5d554626c0fe closed 9 years ago

edd8e884-f507-429a-b577-5d554626c0fe commented 9 years ago

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

CC: @kiwifb @strogdon @kcrisman @gagern @novoselt

Component: packages: standard

Author: Thierry Monteil, François Bissey

Branch/Commit: 9dc580d

Reviewer: Steven Trogdon

Issue created by migration from https://trac.sagemath.org/ticket/17618

kiwifb commented 9 years ago

scipy nearest neighbor - sage 6.5.beta5

kiwifb commented 9 years ago
comment:43

Attachment: SP_nn.jpg

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

kiwifb commented 9 years ago
comment:44

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.

kiwifb commented 9 years ago
comment:45

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
kiwifb commented 9 years ago

Changed commit from 065dc77 to b2c0045

kiwifb commented 9 years ago

Changed branch from u/tmonteil/MPL-1.4 to u/fbissey/trac17618-MPL-1.4

kiwifb commented 9 years ago
comment:46

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.

jdemeyer commented 9 years ago
comment:48

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?

kiwifb commented 9 years ago
comment:49

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.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:51

Is anything happening with this ticket? The broken 'source code' links still appear on our online doc.

Nathann

kiwifb commented 9 years ago
comment:52

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.

dimpase commented 9 years ago
comment:53

test, please ignore

kiwifb commented 9 years ago
comment:54

OK we need to move this forward. I suggest and will try to do in the next few days:

We'll probably need to deal with Delaunay in MPL 2.0 but there may be suitable replacement in that version.

kiwifb commented 9 years ago

Changed branch from u/fbissey/trac17618-MPL-1.4 to u/fbissey/MPL1.4.3

kiwifb commented 9 years ago

Changed author from Thierry Monteil to Thierry Monteil, François Bissey

kiwifb commented 9 years ago

Description changed:

--- 
+++ 
@@ -3,4 +3,4 @@
 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:
-[https://downloads.sourceforge.net/project/matplotlib/matplotlib/matplotlib-1.4.2/matplotlib-1.4.2.tar.gz](https://downloads.sourceforge.net/project/matplotlib/matplotlib/matplotlib-1.4.2/matplotlib-1.4.2.tar.gz)
+[https://downloads.sourceforge.net/project/matplotlib/matplotlib/matplotlib-1.4.3/matplotlib-1.4.3.tar.gz](https://downloads.sourceforge.net/project/matplotlib/matplotlib/matplotlib-1.4.3/matplotlib-1.4.3.tar.gz)
kiwifb commented 9 years ago

Changed commit from b2c0045 to b16e00b

kiwifb commented 9 years ago
comment:55

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
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

a62644fSlim the tarball by removing images. Also commit correct version.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from b16e00b to a62644f

kiwifb commented 9 years ago

Description changed:

--- 
+++ 
@@ -3,4 +3,4 @@
 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:
-[https://downloads.sourceforge.net/project/matplotlib/matplotlib/matplotlib-1.4.3/matplotlib-1.4.3.tar.gz](https://downloads.sourceforge.net/project/matplotlib/matplotlib/matplotlib-1.4.3/matplotlib-1.4.3.tar.gz)
+[http://www.lmona.de/files/sage/matplotlib-1.4.3.tar.bz2](http://www.lmona.de/files/sage/matplotlib-1.4.3.tar.bz2)
kiwifb commented 9 years ago
comment:57

Ready for review.

New commits:

a62644fSlim the tarball by removing images. Also commit correct version.
edd8e884-f507-429a-b577-5d554626c0fe commented 9 years ago

Changed branch from u/fbissey/MPL1.4.3 to u/tmonteil/MPL1.4.3

edd8e884-f507-429a-b577-5d554626c0fe commented 9 years ago
comment:59

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.
edd8e884-f507-429a-b577-5d554626c0fe commented 9 years ago

Changed commit from a62644f to 4612049

strogdon commented 9 years ago
comment:60

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

kiwifb commented 9 years ago
comment:61

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?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

30fb859#17618 fix typo in discrete_gaussian_lattice.py
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 4612049 to 30fb859

kiwifb commented 9 years ago
comment:63

Cool, back to need_review.

strogdon commented 9 years ago

Reviewer: Steven Trogdon

strogdon commented 9 years ago
comment:64

This works for me.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:65
+[ -n "${SAGE_ROOT}" ] || SAGE_ROOT="$(pwd)/../../../"

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

strogdon commented 9 years ago
comment:66

Replying to @nathanncohen:

+[ -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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

32afbe1#17618 : add some information on how to use spkg-src.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 30fb859 to 32afbe1

edd8e884-f507-429a-b577-5d554626c0fe commented 9 years ago
comment:68

Replying to @nathanncohen:

+[ -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:

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:69

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?

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:70
  • i would add perfect reproductibility of distributed tarballs, but it seems that nobody cares.

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

jhpalmieri commented 9 years ago
comment:71

Replying to @nathanncohen:

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.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:72

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.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:73

Oh. You are right, it totally breaks when it's the first line of a bash script.

jhpalmieri commented 9 years ago
comment:74

Replying to @nathanncohen:

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.

strogdon commented 9 years ago
comment:75

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 32afbe1 to 9dc580d

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

9dc580d#17618 spkg-src can be run from anywhere.
edd8e884-f507-429a-b577-5d554626c0fe commented 9 years ago
comment:77

Replying to @strogdon:

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

strogdon commented 9 years ago
comment:78

Replying to @sagetrac-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.

vbraun commented 9 years ago

Changed branch from u/tmonteil/MPL1.4.3 to 9dc580d