image dimensions for show() are in inches

[sagetrac-mvngu]

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]?

Upstream: None of the above - read trac for reasoning.

CC: @kcrisman

Component: graphics

Keywords: image dimensions, figsize

Author: Emily Chen, Punarbasu Purkayastha, Karl-Dieter Crisman

Branch: 4c5b581

Reviewer: Karl-Dieter Crisman, Punarbasu Purkayastha, Jeroen Demeyer

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

[sagetrac-emchennyc]

comment:3

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?

[sagetrac-emchennyc]

Author: emchennyc

[kcrisman]

Reviewer: Karl-Dieter Crisman

[kcrisman]

comment:5

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.

[kcrisman]

Changed author from emchennyc to Emily Chen

[sagetrac-emchennyc]

comment:6

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!

[sagetrac-emchennyc]

Upstream: None of the above - read trac for reasoning.

[ppurka]

comment:7

This ticket is awesome! Brought my laptop to a crawl! :)

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

[kcrisman]

comment:8

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

[sagetrac-emchennyc]

comment:9

@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

[kcrisman]

comment:10

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

[ppurka]

comment:11

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

[sagetrac-emchennyc]

Attachment: trac_5956_figsize_units.patch.gz

[ppurka]

Attachment: trac_5956_figsize_units.1.patch.gz

apply only this

[ppurka]

Attachment: only_for_review.patch.gz

not to be merged. only for review.

[ppurka]

comment:12

Updated the patch to sage-5.7beta0 and introduced some other changes.

The patch attachment: 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

[kcrisman]

Changed branch from u/ppurka/ticket/5956 to u/kcrisman/figsize

[kcrisman]

Changed commit from a756268 to 47c705c

[jdemeyer]

comment:32

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.

[jdemeyer]

comment:34

A minor point, but I wouldn't write segfaulting examples. Imagine the user tries those "examples" without reading the surrounding text...

[ppurka]

comment:35

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.

[kcrisman]

comment:36

That sounds very reasonable.

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

[sagetrac-git]

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

9e7a993	Merge branch 'develop' into figsize
1666659	Final fixes for figsize

[sagetrac-git]

Changed commit from 47c705c to 1666659

[kcrisman]

comment:38

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:

9e7a993	Merge branch 'develop' into figsize
1666659	Final fixes for figsize

[jdemeyer]

Changed reviewer from Karl-Dieter Crisman, Punarbasu Purkayastha to Karl-Dieter Crisman, Punarbasu Purkayastha, Jeroen Demeyer

[jdemeyer]

comment:39

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

[jdemeyer]

comment:40

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

[sagetrac-git]

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

4c5b581

More updates to figsize logic

[sagetrac-git]

Changed commit from 1666659 to 4c5b581

[kcrisman]

comment:42

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.

[ppurka]

comment:43

I think you have addressed Jeroen's concerns.

[vbraun]

Changed branch from u/kcrisman/figsize to 4c5b581

[kcrisman]

comment:45

Followup #17057.

[kcrisman]

Changed commit from 4c5b581 to none