-
-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rework save/show in plot, use Matplotlib's axes code, upgrade matplotlib #5448
Comments
Attachment: plot.patch.gz |
comment:2
mhansen: are you still working on this, or are you posting it in hopes that someone else will work on it? |
comment:3
I was planning on finishing this up in the near future. I posted it because I wanted to see what all I had left to do and possibly to get feedback from others. |
comment:4
Here's another project that provides nice axes functionality for matplotlib, for reference: |
comment:5
Just a very naive question: It seems like a lot of work to include Matplotlib's functionality in plot(). Wouldn't it be easier to just transfer the list of values generated by plot() to Matplotlib? Then, the rest could be done with the conventional Matplotlib commands. So far, I have been able to do everything I wanted using Matplotlib, the only impasse being the generation of points to plot, which I believe is solved in plot() a lot more elegantly than just evaluating a number of equidistant points. I apologise in advance for my ignorance. |
comment:6
PLEASE KEEP MIKE'S PATCH ATTACHED TO THIS TICKET FOR NOW (I think he might have some more elegant ways of doing things than I did, which I'd like to copy to my patch). Please give feedback! |
This comment has been minimized.
This comment has been minimized.
comment:7
Here is the old description (applicable to Mike's patch, and some of which may be applicable to my patch): This removes the axes.py code from Sage and switches to Matplotlib's code. This allows for things like log scales as well as more flexibility in controlling how the ticks are labeled formatted. It also fixes a number of existing bugs (like 2754). After this change, it will be trivial to add a viewer='flot' option to Graphics. The patch still needs some work a.k.a. doctests. There are also a few things that don't work yet (like tick color and tick fontsize), and matrix_plot needs a matplotlib Locater and Formatter. Also, GraphicsArray needs to be updated and should get lots of doctests for it added. |
comment:8
Just a question to Jason - will this break any currently existing code that has rgbcolor instead of color in use? I assume not but thought I would ask - ditto for any other options for plot or show that would be deprecated? |
comment:9
Replying to @kcrisman:
I think the only code it might break is the use of rgbcolor in gridline styles, which according to the docs, should not have been allowed anyway (matplotlib lines do not have an rgbcolor option). For everything else, I've been careful to doctest everything in plot.py, and all tests pass. |
comment:10
Okay, first off patch needs work if only because it does not apply to 4.1.1! Fix is to correct all spellings of "apect" in the patch to "aspect" and to replace one instance of "png's" at the end to "PNG's". Also, eventually presumably axes.py should go to dev/null, right? I also get a bunch of deprecation warnings, but presumably that is known. As to the stuff itself:
But great work notwithstanding! Looking forward to whatever comes out of this ticket, and the mpl upgrade seems nice. |
comment:11
Thanks for looking at this! I haven't had time to come back to it yet (our semester started...) Replying to @kcrisman:
On the one hand, I can turn off clipping. However, that tends to make really ugly plots when you have frame axes (since then the dots go outside of the frames). On the other hand, we can automatically enlarge the plot axes (so that if you request -3..3, it might actually cover -3.5..3.5). This could be frustrating, but is sort of what happens now. I guess you have to make a choice between things extending beyond your plot (no clipping) or extending your plot. For right now, I was hoping that people would just make their ranges a bit bigger by hand.
Yeah, it's a bit different, but after playing with it for a while, I liked it. At a glance, it oriented me to what I was looking at and how it was compared to the infinite plane. This is definitely something that should go up for a vote. Also, I'd like to add an option for a custom crossing point. That would be another 5 lines of code, maybe.
For right now, I automatically expand things to not clip axes labels. There might be a bug in that. Can you post an example?
It would be easy to omit one or both of these zeros in these cases.
Okay, I'll try to look at this.
and your proposed fix is...?
Totally easy. You have the full power of matplotlib at your command. You just do something like: p=plot(m).matplotlib() Now just do custom axes locators for axes in p, like in http://matplotlib.sourceforge.net/examples/pylab_examples/custom_ticker1.html When you are done, just do: p.save_fig('test.png') # or something like this or (with another change that should be coming soon, now that I understand matplotlib a lot better): Graphics(p) # This is now a sage Graphics object... |
comment:12
Replying to @jasongrout:
Definitely should be automatic, I think, if it's already-existing behavior.
I'm not sure what you are looking at. The axes do not actually cross! That is bad, imho.
Yeah, that would be good, though hopefully not often needed.
Something to think about. Presumably the other labels would make it clear it's a zero.
Using frames again for slope fields (the previous behavior). |
comment:13
Replying to @jasongrout:
It appears to have something to do with where the labels are located.
is fine, but
yields the problem. This also happens with point sets:
Back to the other issues, note that while the following does plot
it also is not ideal, as the axes don't even come close to touching. By the way, the pointsize is irrelevant - even with no pointsize given, the points are still cut off. Also, there is a mysterious cut-off thing at the bottom which look like +1.00e3, but it's cut off so I can't tell for sure. Any ideas on that? |
comment:14
Replying to @kcrisman:
Okay, I think I have an idea that will do this (find the bounding box of the scatter plots, convert those coordinates to axes coordinates, and then enlarge the axes to include those bounding boxes)
And, after I played with it for a bit, I thought it was great! The axes only ever cross at the origin. That is wonderfully refreshing and consistent, and leads to being able to immediately orient yourself in the graph without having to examine and think about the axes tick labels. If there is some space, then that means you are way above the axis (and the side the axis is on tells you where the axis really is). I think of it sort of like a small zigzag break in the axes. Maybe if we put that in an explicit small zigzag symbol, would that help you?
Ah, I thought I did this. I did it for contour plots; doing it for vector fields is about a one-line change (just add frame=True to the |
comment:15
I see. The problem is that this is just as non-standard as the previous behavior, which people also complained about. For sure it shouldn't happen with
I do agree that it is more consistent, if you can get it to be consistent - and if it is REALLY well documented, i.e. right up at the top of the plot docs and in the tutorial. If you think the rest of the stuff is ready for prime time, you should definitely put the various versions in screenshots up for a vote on sage-devel, since improved axes would really be great to have. |
comment:16
Replying to @kcrisman:
I figured out what the problem was. I'm setting the x-axis label y-coordinate to y=0, which is obviously wrong if y=0 is not in the picture. I'm doing a similar thing with the y-axis label. I've emailed the matplotlib list for help on getting the right matplotlib transform to get the label to be offset from the actual axis in the picture. |
comment:17
The new patch:
|
comment:18
A minor question about #260: When I rebase its patch, should I change this part:
? |
comment:19
Sorry about the dumb question, but how do I apply this patch? Following the instructions at http://www.sagemath.org/doc/developer/producing_patches.html, I did: However, this launched me straight into editing the patch, which I was not really up to. Exiting the editors led to the following message: RuntimeError: Refusing to do operation since you still have unrecorded changes. You must check in all changes in your working repository first. Thanks for your help! Stan |
comment:39
I can't figure out how to make transparent=True work (some pics would be nice for you to post). But it doesn't break anything, so based on the devel discussion, let's get this in - everything works more or less as well as I can help right now. Things to think about: Note there are some mpl warnings about numerix and other deprecated things (e.g., legend.handlelen) when you start up the notebook. Make sure that #260 works off this patch, as mpatel has indicated willingness to do. Note to release manager: needs both new matplotlib AND new networkx spkgs in order to avoid many, many deprecation warnings. |
comment:40
It seems that the problem of cutting off axes labels of large numbers is just a matter of when the axis switches to scientific notation. Consider this:
Only numbers > 1.e+11 are converted to scientific notation, while the smaller ones get cut off. This could be fixed by changing to scientific notation at much smaller numbers. Actually, the change from decimal to scientific notation half way across the axis is something that bothered me in MMA already. Could you either let the user decide which notation to use or make it the same for the whole axis, depending on whether the largest (or smallest) number requires it? Just a few ideas for when you get to fixing the matplotlib tick stuff. Cheers |
comment:41
Both of you, please make sure you are using the most recent version of the patch (from around the time I posted to sage-devel). Sorry for the rapid iteration---your feedback has very helpful.
Of course, in a web browser, you will "see" a white background through the image. If you save the image, you can "see" the transparent background. Your viewer may not be showing you the transparency easily.
|
comment:42
Oh, based on the problems I mentioned in the sage-devel thread, I'm not quite comfortable with this going in yet, unless there's an understanding that there is a fix-up patch before the next release. So I'm changing this back to "needs work" for now. I hope that by the weekend, the issues will be resolved. |
comment:43
And one more thing needs to be done: documentation! (for the new functions added, and check the documentation for functions that were changed). |
This comment has been minimized.
This comment has been minimized.
Attachment: trac-5448-matplotlib-axes-gridlines.2.patch.gz apply instead of previous patches |
Attachment: trac-5448-matplotlib-axes-gridlines.3.patch.gz |
comment:45
I am deleting the following code from the patch because it is not used right now. If in the future we want to have more control over the size of the figure, but still want to automatically shrink the plot to fit everything (labels, etc.) inside of the figure without clipping them, then the following code will prove useful. I'm pasting the code here so that it is some sort of permanent record. This is an extension of an idea in the matplotlib FAQ.
|
Attachment: trac-5448-matplotlib-axes-gridlines.4.patch.gz apply instead of previous patches |
This comment has been minimized.
This comment has been minimized.
comment:46
I believe I've taken care of all outstanding issues, so this patch is ready to be (re-)reviewed. |
comment:47
I've moved Mike's plot.patch to #6893, so that patch can be deleted from this ticket. In fact, all patches except for trac-5448-matplotlib-axes-gridlines.4.patch can be deleted from this ticket. |
comment:48
Positive review - behaves as advertised after actually viewing many, many of the plots in the sage/plot/ directory, and testing on a few others which had given trouble before. I look forward to doing custom axes soon; perhaps that should be wrapped, and then a few other tickets could be closed. These might include (not just axes-related, and not necessarily, but worth checking into) #6548, #5645, #5128, #4689, #4384, #4194, #2900, #2189, and #1431. Good things; transparent definitely DOES now work, complex plots and matrix plots look better because they're "set off" a bit, etc. Here are things to address in a follow-up ticket, at least maybe: Contour plot - if fill=False and contours are grayscale, the axes could be misinterpreted Contour plot - show(axes=False) and show(axes=True) seem to be identical on the last example Plotting - how well documented is the new axis behavior, where it does NOT intersect? This should be clear, e.g. the Riemann zeta example in plot.py looks funny, until you realize it's from 1 to 27. It still seems weird to me when it's that close, but I suppose it's okay as long as it is very very clear in documentation. Axis labels - should point out difference between ['x','y'] and ['$x$','$y$']. Some people might not like the LaTeXed version When scientific notation comes into play is not always clear, and should be in the documentation - compare plot(x2, 490,500) and plot(x2,-490,500), which have the same "height" but only one gets e, presumably since it covers a larger range |
Merged: Sage 4.1.2.alpha1 |
Reviewer: Karl-Dieter Crisman |
comment:49
Merged |
This makes the axes and gridline code in Sage obsolete and upgrades the matplotlib spkg.
This patch depends on the following spkgs.
http://sage.math.washington.edu/home/jason/matplotlib-0.99.0.spkg (for all of the good stuff for this patch)
http://sage.math.washington.edu/home/jason/networkx-0.99.p1-fake_really-0.36.p1.spkg (to get rid of lots of deprecation warnings from the upgraded matplotlib).
Last patch applies to 4.1.2.alpha0, or 4.1.1 with #6685.
Doctests in plot/*.py pass. -docbuild reference html passes.
CC: @sagetrac-wcauchois @kcrisman
Component: graphics
Author: Jason Grout
Reviewer: Karl-Dieter Crisman
Merged: Sage 4.1.2.alpha1
Issue created by migration from https://trac.sagemath.org/ticket/5448
The text was updated successfully, but these errors were encountered: