#5956 closed defect (fixed)
image dimensions for show() are in inches
Reported by: | mvngu | Owned by: | was |
---|---|---|---|
Priority: | minor | Milestone: | sage-6.4 |
Component: | graphics | Keywords: | image dimensions, figsize, beginner |
Cc: | kcrisman | Merged in: | |
Authors: | Emily Chen, Punarbasu Purkayastha, Karl-Dieter Crisman | Reviewers: | Karl-Dieter Crisman, Punarbasu Purkayastha, Jeroen Demeyer |
Report Upstream: | None of the above - read trac for reasoning. | Work issues: | |
Branch: | 4c5b581 (Commits) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
As discussed at this sage-devel thread, the optional argument figsize
of the command show()
needs to clearly state that the units of the image are in inches. As of Sage 3.4.1, the docstring for show()
says:
- ``figsize``- [width, height] (same for square aspect)
which can be interpreted to mean that one can do something like figsize=[w,h]
. But something like the following produces a segmentation fault:
[mvngu@sage ~]$ sage ---------------------------------------------------------------------- | Sage Version 3.4.1, Release Date: 2009-04-21 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- sage: q = var("q") sage: f(q) = (q^4 - q^2 + 1) * (q^4 + q^3 + q^2 + q + 1) * (q^4 - q^3 \ + q^2 - q + 1) * (q^6 + q^5 + q^4 + q^3 + q^2 + q + 1) * (q^6 - q^5 + \ q^4 - q^3 + q^2 - q + 1) * (q^(20) - q^(18) - q^(14) - q^(12) + q^(10) \ - q^8 - q^6 - q^2 + 1) sage: g(q) = q^8 * (q^4 + q^2 + 1)^2 * (q^4 + 1)^5 sage: p = complex_plot(f/g, (-2,2), (-2,2)) sage: p.show(figsize=[256,256])
while the following results in a ValueError
:
---------------------------------------------------------------------- | Sage Version 3.4.1, Release Date: 2009-04-21 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- sage: q = var("q") sage: f(q) = (q^4 - q^2 + 1) * (q^4 + q^3 + q^2 + q + 1) * (q^4 - q^3 \ + q^2 - q + 1) * (q^6 + q^5 + q^4 + q^3 + q^2 + q + 1) * (q^6 - q^5 + \ q^4 - q^3 + q^2 - q + 1) * (q^(20) - q^(18) - q^(14) - q^(12) + q^(10) \ - q^8 - q^6 - q^2 + 1) sage: g(q) = q^8 * (q^4 + q^2 + 1)^2 * (q^4 + 1)^5 sage: p = complex_plot(f/g, (-2,2), (-2,2)) sage: p.show(figsize=[500,500]) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) ... ValueError: width and height must each be below 32768
Essentially, the documentation for show()
needs to be updated, especially the optional arguments, to clearly explain the units of measurement of the width and height of the image size. Also, it would be a good idea to specify how one can pass in values for those dimensions. For example, can one do this figsize=[124,124]
?
Attachments (4)
Change History (49)
comment:1 Changed 11 years ago by
- Keywords beginner added
- Report Upstream set to N/A
comment:2 Changed 11 years ago by
- Cc kcrisman added
comment:3 Changed 8 years ago by
- Status changed from new to needs_review
comment:4 Changed 8 years ago by
comment:5 Changed 8 years ago by
- Reviewers set to Karl-Dieter Crisman
- Status changed from needs_review to needs_work
This would be a good start.
What's the reference for the dpi, just out of curiosity? (I assume it's right in the Sage or mpl source.) Also, telling us the maximum possible would be helpful, given the errors - especially since the one above is confusing since the 32768
is in "dots", not inches. Presumably if one reset the default dpi...
sage: P.show(figsize=[1,327],dpi=100) sage: P.show(figsize=[1,328],dpi=100) <ValueError> sage: P.show(figsize=[1,328],dpi=80)
So this should be very, very clear, in order to resolve this ticket.
Additionally, you'll want to add a doctest to verify that some things don't go boom, and that the error is correctly raised, etc. Maybe even a # not tested
line with the segfault.
Finally, we may also want to report the segfault upstream with a "pure" matplotlib example, if you can concoct one.
But all that said, thanks very much for looking at this - this is just part of the normal review process, good start.
comment:6 Changed 8 years ago by
- Report Upstream changed from N/A to None of the above - read trac for reasoning.
- Status changed from needs_work to needs_review
Thank you for the pointers. In matplotlib/rcsetup.py the default figure properties start at line 508. line 509 # figure size in inches: width by height 'figure.figsize' : [ [8.0,6.0], validate_nseq_float(2)] line 523 'savefig.dpi' : [100, validate_float], # DPI
I submitted a new patch to include what I think you mean by a doctest. I wasn't sure what you meant about including '# not tested line with the segfault'. Should I include an example with the segfault in the docstring?
Strangely enough, I did not raise any errors when I tried a 'pure' matplotlib example...
from matplotlib import pyplot as plt f=plt.figure(figsize=[28,10],dpi=100) f.show()
Would someone kindly review and let me know what else should be included in the doctest? I appreciate your patience with this, as I am (obviously) new and hoping to learn. Thank you!
comment:7 Changed 8 years ago by
This ticket is awesome! Brought my laptop to a crawl! :)
@emchennyc: I might come back to this ticket if kcrisman doesn't. For now, it seems that matplotlib (even the latest 1.2.0 release) does not handle the parameters [256,256]
properly. It tries to allocate memory and then fails, resulting in a segfault.
What kcrisman asks is a doctest like this (under a TEST:
section; look at how these sections are written in other functions or other files):
TEST: The following plot segfaults sage and should not be doctested.:: sage: plot(x).show(figsize=[256,256]) # not tested
If you do happen to run this command, be very careful. I suggest you set a ulimit before you start sage. If you have, say 3G, of memory/RAM, then set ulimit to 2G and only then start sage. If you have even less memory then you will need to set even lower ulimit, but sage won't run properly below 1G.
$ ulimit -v 2000000 $ sage sage: plot(x).show(figsize=[256,256]) # boom, segv!!
That said, I would like you to set some defaults in your editor. Currently, your patch contains a mixture of tabs and spaces. I suggest you change the settings in your editor to not insert tabs, and instead insert 4 spaces every time you press a tab key.
comment:8 Changed 8 years ago by
- Status changed from needs_review to needs_work
Needs work due to the issues raise by ppurka, but getting close!
comment:9 follow-up: ↓ 10 Changed 8 years ago by
@ppurka Thank you for the feedback! The tests I included in the attached patch pass when I run: ./sage -t -verbose 'devel/sage-5956/sage/plot/graphics.py'
Then, I
- ran ./sage -docbuild reference html
- navigated to SAGEROOT/devel/branch/doc/output/html/en/reference
- opened plotting.html in a browser
- visited the doc page for Graphics objects
but did not see the documentation that I added. Did I miss a step?
Also, here is a matplotlib example which I believe this bug is related to. Please correct me if I'm wrong and feel free to advise if I should use this example to report as a matplotlib issue: http://pastebin.com/raw.php?i=5Arg52e2
comment:10 in reply to: ↑ 9 Changed 8 years ago by
Replying to emchennyc:
@ppurka Thank you for the feedback! The tests I included in the attached patch pass when I run: ./sage -t -verbose 'devel/sage-5956/sage/plot/graphics.py'
Then, I
You have to do ./sage -b first. The tests run on the copy of the code in the devel/sage directory, but the docs build off the ones in the build directory which that command does.
- ran ./sage -docbuild reference html
- navigated to SAGEROOT/devel/branch/doc/output/html/en/reference
- opened plotting.html in a browser
- visited the doc page for Graphics objects
but did not see the documentation that I added. Did I miss a step?
Also, here is a matplotlib example which I believe this bug is related to. Please correct me if I'm wrong and feel free to advise if I should use this example to report as a matplotlib issue: http://pastebin.com/raw.php?i=5Arg52e2
comment:11 Changed 8 years ago by
Thanks for the update. There is one more thing that I overlooked earlier. The documentation should mention that the number 32768
is in dots per inch. Maybe the text of ValueError
should also end in 32768 dots per inch.
sage: e.show(figsize=[328,10],dpi=100) ValueError: width and height must each be below 32768 dots per inch.
I think the following modification to the figsize
documentation is warranted.
- ``figsize`` - (default: [8.0,6.0]) [width, height] inches. The maximum value of each of the width and the height can be 327 inches, at the default ``dpi`` of 100 dpi, which is just shy of the maximum allowed value of 32768 dots per inch.
Then, the test should have the following description:
The figsize width and height parameters must be less than 328 inches each, corresponding to the maximum allowed dpi of 32768.::
This will make the number 32768 more clear to anyone who is curious what that number means in the ValueError
.
I am unable to replicate your matplotlib example. A figure doesn't seem to have a show()
attribute. However, the following example successfully crashes matplotlib.
~» ulimit -v 2000000 ~» sage -ipython Python 2.7.3 (default, Dec 30 2012, 21:34:30) Type "copyright", "credits" or "license" for more information. IPython 0.10.2 -- An enhanced Interactive Python. ? -> Introduction and overview of IPython's features. %quickref -> Quick reference. help -> Python's own help system. object? -> Details about 'object'. ?object also works, ?? prints more. In [1]: from matplotlib import pyplot In [2]: pyplot.figure(figsize=[232,232]) Out[2]: <matplotlib.figure.Figure object at 0x12ba950> In [3]: pyplot.savefig('/tmp/a.png') ... RuntimeError: Could not allocate memory for image
Changed 8 years ago by
comment:12 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Updated the patch to sage-5.7beta0 and introduced some other changes.
The patch only_for_review.patch contains the changes I made after rebasing the patch to 5.7beta0. In particular, I have introduced the changes I suggested in my comments, and have fixed failing doctests due to the use/redefining of e
in the earlier patch.
The changes needs review.
Patchbot apply trac_5956_figsize_units.1.patch
comment:13 Changed 8 years ago by
- Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Punarbasu Purkayastha
comment:14 Changed 8 years ago by
This hit me today in class! At least, I think so - figsize=[5,5]
shouldn't necessarily yield this, but it was the same message. I thought it was in, but high priority now.
Also, I think that #12592 is a dup.
comment:15 Changed 8 years ago by
Oh, I figured out my (very stupid) problem - I used figsize=[-5,5]
, which is of course absurd. Still, probably wouldn't hurt to put in a check for absurd values... such typos are "all too easy".
comment:16 Changed 8 years ago by
Needs a bit of work. The limit is in pixels (dots), not dots per inch. 2^15
dots per inch would be impressive indeed. Notice also that apparently this could be raised if one really wanted to.
I'll add a reviewer patch momentarily.
Changed 8 years ago by
comment:17 follow-up: ↓ 18 Changed 8 years ago by
- Description modified (diff)
I think this needs some review, since I add some assertions and so forth. I hope this clarifies things more, though, and adds doc in the main plot page where it is probably needed.
ppurka, if you are an author too, feel free to add yourself. Positive review to everything before my patch.
Patchbot: apply trac_5956_figsize_units.1.patch and trac_5956-reviewer.patch to devel/sage.
comment:18 in reply to: ↑ 17 Changed 8 years ago by
- Status changed from needs_review to positive_review
Replying to kcrisman:
I think this needs some review, since I add some assertions and so forth. I hope this clarifies things more, though, and adds doc in the main plot page where it is probably needed.
Thanks. Looks good to me.
ppurka, if you are an author too, feel free to add yourself. Positive review to everything before my patch.
I did change some doctests.
comment:19 follow-up: ↓ 20 Changed 8 years ago by
- Status changed from positive_review to needs_work
Use ValueError
for bad user input, not AssertionError
. If there is any way to produce an AssertionError
using public functions, that is by definition a bug.
Assertions are meant to check internal consistency inside algorithms. They express "I know this condition is true", not "I want to check that this condition is true".
comment:20 in reply to: ↑ 19 ; follow-up: ↓ 21 Changed 8 years ago by
Assertions are meant to check internal consistency inside algorithms. They express "I know this condition is true", not "I want to check that this condition is true".
Okay, fair enough. The problem is that now I have to go remember how to check that something is a numeric type as well - which is better in the long run, but which is tricky with all the zillions of numeric input types Sage has... does something like isnumeric
work? It sounds familiar.
comment:21 in reply to: ↑ 20 Changed 8 years ago by
Replying to kcrisman:
Assertions are meant to check internal consistency inside algorithms. They express "I know this condition is true", not "I want to check that this condition is true".
Okay, fair enough. The problem is that now I have to go remember how to check that something is a numeric type as well
Just convert, for example
x = float(x)
comment:22 follow-up: ↓ 32 Changed 8 years ago by
Why should we go around checking for numeric types? This is not a "critical" application that should ensure sane inputs for all kinds of inputs. If the user sends in garbage, they will eventually get garbage out from at least matplotlib, if not earlier in the process.
comment:23 Changed 8 years ago by
It's not critical in that sense, but it's the kind of thing where I figure we might as well make sure that any error message sent makes sense. The current error
ValueError: width and height must each be below 32768
doesn't refer to figsize
and at least made me totally miss that I had somehow smuggled a minus sign in there, since I was frantically trying to make the image smaller and wondering why five was too big :-) I figure if it can happen to an experienced Sage user "on the spot", it will definitely happen to a new user. Jeroen's comment seems quite reasonable especially as everything matplotlib is going to end up a float anyway, if I recall correctly.
I'll try to get this all packaged up in the next day or two.
comment:24 Changed 7 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:25 Changed 7 years ago by
- Branch set to u/ppurka/ticket/5956
- Modified changed from 08/13/13 15:35:53 to 08/13/13 15:35:53
comment:26 follow-up: ↓ 30 Changed 7 years ago by
Added a git version of the patch. This is based on 6.1.beta5.
comment:27 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:28 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:29 Changed 6 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:30 in reply to: ↑ 26 Changed 6 years ago by
- Commit set to a756268ebc83500dcb785204d95901b711e38561
I'll try to get this all packaged up in the next day or two.
Next day or two, next year or two...
Added a git version of the patch. This is based on 6.1.beta5.
Amazingly, apparently still applies.
But we still haven't taken care of Jeroen's comment. So I will try to do that now.
New commits:
dcbcd73 | Trac #5956 Graphics object show() doc explains figsize dimension is inches; added doctest for max figsize and figsize values that cause segfault
|
a756268 | Trac 5956 additional patch adding doc, tests, and assertion for figsize
|
comment:31 Changed 6 years ago by
- Branch changed from u/ppurka/ticket/5956 to u/kcrisman/figsize
- Commit changed from a756268ebc83500dcb785204d95901b711e38561 to 47c705c6ee6ab969ec70eb6ed1628e56b59042fe
- Status changed from needs_work to needs_review
comment:32 in reply to: ↑ 22 Changed 6 years ago by
Replying to ppurka:
Why should we go around checking for numeric types? This is not a "critical" application that should ensure sane inputs for all kinds of inputs. If the user sends in garbage, they will eventually get garbage out from at least matplotlib, if not earlier in the process.
Well, it could be that the user will send some Sage numeric type, which matplotlib might treat like garbage. A conversion like
x = float(x) # assuming that it's a float that you need
will solve both problems: it will raise a TypeError
when true garbage is given but it should work for all Sage numeric types.
comment:33 Changed 6 years ago by
- Description modified (diff)
comment:34 follow-up: ↓ 35 Changed 6 years ago by
A minor point, but I wouldn't write segfaulting examples. Imagine the user tries those "examples" without reading the surrounding text...
comment:35 in reply to: ↑ 34 Changed 6 years ago by
Replying to jdemeyer:
A minor point, but I wouldn't write segfaulting examples. Imagine the user tries those "examples" without reading the surrounding text...
How about we prepend the segfaulting line with a comment? Like this:
sage: #p.show(figsize=[232,232],dpi=100) # not tested
So, anyone who blindly copy pastes will get nothing out of it. And has to make two mistakes to make it segfault. I think the reason this example is there is to make the user aware that unreasonable parameters will result in an uncomfortable end.
comment:36 Changed 6 years ago by
That sounds very reasonable.
Jeroen, are you saying that my latest commit needs something added like the float
business?
comment:37 Changed 6 years ago by
- Commit changed from 47c705c6ee6ab969ec70eb6ed1628e56b59042fe to 16666594c6d72b767adc1f2e9e7118469ffa600a
comment:38 Changed 6 years ago by
comment:39 Changed 6 years ago by
- Reviewers changed from Karl-Dieter Crisman, Punarbasu Purkayastha to Karl-Dieter Crisman, Punarbasu Purkayastha, Jeroen Demeyer
- Status changed from needs_review to needs_work
Is there a check that len(figsize) == 2
?
I would change the logic of those branches to
if figure is None: # add a good comment here ... elif isinstance(figsize, (list, tuple)): # figsize should be two positive numbers if len(figsize) != 2: raise ValueError('...') ... else: # figsize should be a single positive number figsize = float(figsize) # to pass to mpl if figsize <= 0: raise ValueError("figsize should be positive, not {0}".format(figsize)) ...
And add a doctest for some non-float figsize
argument like
sage: var('x') sage: ...figsize=x...
comment:40 Changed 6 years ago by
The Unhandled SIGSEGV:
message isn't the current message (which is shorter).
comment:41 Changed 6 years ago by
- Commit changed from 16666594c6d72b767adc1f2e9e7118469ffa600a to 4c5b581fb521519cdd353bc40445b4f7b4589862
Branch pushed to git repo; I updated commit sha1. New commits:
4c5b581 | More updates to figsize logic
|
comment:42 Changed 6 years ago by
- Status changed from needs_work to needs_review
Okay, that's what I can do today. If you really want the code reorganized that badly it will have to fall to you, I'm afraid. But everything else should be taken care of.
comment:43 Changed 6 years ago by
- Status changed from needs_review to positive_review
I think you have addressed Jeroen's concerns.
comment:44 Changed 6 years ago by
- Branch changed from u/kcrisman/figsize to 4c5b581fb521519cdd353bc40445b4f7b4589862
- Resolution set to fixed
- Status changed from positive_review to closed
comment:45 Changed 6 years ago by
- Commit 4c5b581fb521519cdd353bc40445b4f7b4589862 deleted
Followup #17057.
Made a doc update after reading this thread: https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/xBElS3vAu5c
Do you think any more info should be included in the changes?