Opened 8 years ago

Closed 3 years ago

Last modified 3 years ago

#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 jdemeyer)

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)

trac_5956_figsize_units.patch (3.5 KB) - added by emchennyc 5 years ago.
trac_5956_figsize_units.1.patch (3.6 KB) - added by ppurka 5 years ago.
apply only this
only_for_review.patch (4.1 KB) - added by ppurka 5 years ago.
not to be merged. only for review.
trac_5956-reviewer.patch (5.1 KB) - added by kcrisman 5 years ago.

Download all attachments as: .zip

Change History (49)

comment:1 Changed 7 years ago by jason

  • Keywords beginner added
  • Report Upstream set to N/A

comment:2 Changed 7 years ago by kcrisman

  • Cc kcrisman added

comment:3 Changed 5 years ago by emchennyc

  • Authors set to emchennyc
  • Status changed from new to needs_review

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?

Last edited 5 years ago by emchennyc (previous) (diff)

comment:4 Changed 5 years ago by emchennyc

Last edited 5 years ago by emchennyc (previous) (diff)

comment:5 Changed 5 years ago by kcrisman

  • Authors changed from emchennyc to Emily Chen
  • 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 5 years ago by emchennyc

  • 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 5 years ago by ppurka

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 5 years ago by kcrisman

  • Status changed from needs_review to needs_work

Needs work due to the issues raise by ppurka, but getting close!

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

  • 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 5 years ago by kcrisman

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 5 years ago by ppurka

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 5 years ago by emchennyc

Changed 5 years ago by ppurka

apply only this

Changed 5 years ago by ppurka

not to be merged. only for review.

comment:12 Changed 5 years ago by ppurka

  • 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 5 years ago by ppurka

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Punarbasu Purkayastha

comment:14 Changed 5 years ago by kcrisman

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 5 years ago by kcrisman

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 5 years ago by kcrisman

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 5 years ago by kcrisman

comment:17 follow-up: Changed 5 years ago by kcrisman

  • Authors changed from Emily Chen to Emily Chen, Karl-Dieter Crisman
  • 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 5 years ago by ppurka

  • Authors changed from Emily Chen, Karl-Dieter Crisman to Emily Chen, Punarbasu Purkayastha, Karl-Dieter Crisman
  • 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: Changed 5 years ago by jdemeyer

  • 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".

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:20 in reply to: ↑ 19 ; follow-up: Changed 5 years ago by 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 - 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 5 years ago by jdemeyer

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: Changed 5 years ago by 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.

comment:23 Changed 5 years ago by kcrisman

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 4 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:25 Changed 4 years ago by ppurka

  • 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: Changed 4 years ago by ppurka

Added a git version of the patch. This is based on 6.1.beta5.

comment:27 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:28 Changed 3 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:29 Changed 3 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:30 in reply to: ↑ 26 Changed 3 years ago by kcrisman

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

dcbcd73Trac #5956 Graphics object show() doc explains figsize dimension is inches; added doctest for max figsize and figsize values that cause segfault
a756268Trac 5956 additional patch adding doc, tests, and assertion for figsize

comment:31 Changed 3 years ago by kcrisman

  • 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

Okay, the last commit only needs review.


New commits:

c172affMerge branch 'u/ppurka/ticket/5956' of trac.sagemath.org:sage into figsize
47c705cMake ValueError, not assertion

comment:32 in reply to: ↑ 22 Changed 3 years ago by jdemeyer

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 3 years ago by jdemeyer

  • Description modified (diff)

comment:34 follow-up: Changed 3 years ago by jdemeyer

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 3 years ago by ppurka

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 3 years ago by kcrisman

That sounds very reasonable.

Jeroen, are you saying that my latest commit needs something added like the float business?

comment:37 Changed 3 years ago by git

  • Commit changed from 47c705c6ee6ab969ec70eb6ed1628e56b59042fe to 16666594c6d72b767adc1f2e9e7118469ffa600a

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

9e7a993Merge branch 'develop' into figsize
1666659Final fixes for figsize

comment:38 Changed 3 years ago by kcrisman

Okay, I think this is ready for review. The float stuff is actually unnecessary but it can't hurt, since we definitely only want things that can be floats in there.


New commits:

9e7a993Merge branch 'develop' into figsize
1666659Final fixes for figsize

comment:39 Changed 3 years ago by jdemeyer

  • 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 3 years ago by jdemeyer

The Unhandled SIGSEGV: message isn't the current message (which is shorter).

comment:41 Changed 3 years ago by git

  • Commit changed from 16666594c6d72b767adc1f2e9e7118469ffa600a to 4c5b581fb521519cdd353bc40445b4f7b4589862

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

4c5b581More updates to figsize logic

comment:42 Changed 3 years ago by kcrisman

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

Last edited 3 years ago by kcrisman (previous) (diff)

comment:43 Changed 3 years ago by ppurka

  • Status changed from needs_review to positive_review

I think you have addressed Jeroen's concerns.

comment:44 Changed 3 years ago by vbraun

  • Branch changed from u/kcrisman/figsize to 4c5b581fb521519cdd353bc40445b4f7b4589862
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:45 Changed 3 years ago by kcrisman

  • Commit 4c5b581fb521519cdd353bc40445b4f7b4589862 deleted

Followup #17057.

Note: See TracTickets for help on using tickets.