Opened 12 years ago

Closed 6 years ago

Last modified 6 years ago

# image dimensions for show() are in inches

Reported by: Owned by: mvngu was minor sage-6.4 graphics image dimensions, figsize, beginner kcrisman Emily Chen, Punarbasu Purkayastha, Karl-Dieter Crisman Karl-Dieter Crisman, Punarbasu Purkayastha, Jeroen Demeyer None of the above - read trac for reasoning. 4c5b581

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

### comment:1 Changed 11 years ago by jason

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

### comment:2 Changed 11 years ago by kcrisman

• Cc kcrisman added

### comment:3 Changed 8 years ago by emchennyc

• Authors set to emchennyc
• Status changed from new to needs_review
Version 0, edited 8 years ago by emchennyc (next)

### comment:4 Changed 8 years ago by emchennyc

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

### comment:5 Changed 8 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 8 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 8 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 8 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: ↓ 10 Changed 8 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 8 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 8 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
```

apply only this

### Changed 8 years ago by ppurka

not to be merged. only for review.

### comment:12 Changed 8 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 8 years ago by ppurka

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

### comment:14 Changed 8 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 8 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 8 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.

### comment:17 follow-up: ↓ 18 Changed 8 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 8 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: ↓ 20 Changed 8 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 8 years ago by jdemeyer (previous) (diff)

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

• Milestone changed from sage-5.11 to sage-5.12

### comment:25 Changed 7 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: ↓ 30 Changed 7 years ago by ppurka

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

### comment:27 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

### comment:28 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:29 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

### comment:30 in reply to: ↑ 26 Changed 6 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:

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

 ​c172aff `Merge branch 'u/ppurka/ticket/5956' of trac.sagemath.org:sage into figsize` ​47c705c `Make ValueError, not assertion`

### comment:32 in reply to: ↑ 22 Changed 6 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 6 years ago by jdemeyer

• Description modified (diff)

### comment:34 follow-up: ↓ 35 Changed 6 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 6 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 6 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 6 years ago by git

• Commit changed from 47c705c6ee6ab969ec70eb6ed1628e56b59042fe to 16666594c6d72b767adc1f2e9e7118469ffa600a

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

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

### comment:38 Changed 6 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:

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

### comment:39 Changed 6 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 6 years ago by jdemeyer

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

### comment:41 Changed 6 years ago by git

• 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 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 6 years ago by kcrisman (previous) (diff)

### comment:43 Changed 6 years ago by ppurka

• Status changed from needs_review to positive_review

I think you have addressed Jeroen's concerns.

### comment:44 Changed 6 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 6 years ago by kcrisman

• Commit 4c5b581fb521519cdd353bc40445b4f7b4589862 deleted

Followup #17057.

Note: See TracTickets for help on using tickets.