Improve docstring of Axes.imshow #10982

timhoffm · 2018-04-08T00:18:23Z

PR Summary

As part of #10148: Docstring update for Axes.imshow.

The parameter resample was not documented. From the code, I couldn't really find out what it does.
The parameters shape and imlimare unused and should probably be removed (would do that in a separate PR once this is merged).
The parameter filternorm should probably be a bool all throughout the class hirarchy (separate PR as well).
Should we change the parameter X for Z as is the case in matshow? Z makes a bit more sense, however, technically this would be a breaking change if a user has used it as kwarg imshow(X=data)

anntzer · 2018-04-08T00:36:12Z

See #10442 for making filternorm a bool.

ImportanceOfBeingErnest · 2018-04-08T01:28:59Z

In line 5177

By default, the center of the lower left
pixel is at (0, 0) and pixels have a size of (1, 1), i.e. extent
would be (0, columns-1, 0, rows-1).

is incorrect in two ways.

extent is the edges of the pixels. Hence it starts at -0.5 and goes to columns-0.5
There is something tricky about the interplay between extent and origin. origin defaults to upper (I think) and in that case the extent in y direction needs to to be reversed. See following example:

a = np.triu(np.random.randn(5, 5))
a[a==0] = np.nan

fig, (ax,ax2,ax3,ax4) = plt.subplots(ncols=4)


extent_upper=[-0.5,a.shape[1]-0.5,a.shape[0]-0.5,-0.5]   # <-- here y-extent needs to be inverted

ax.imshow(a, cmap="Blues", origin="upper")
ax2.imshow(a, cmap="Reds", origin="upper", extent=extent_upper)

extent_lower=[-0.5,a.shape[1]-0.5,-0.5,a.shape[0]-0.5]

ax3.imshow(a, cmap="Blues", origin="lower")
ax4.imshow(a, cmap="Reds", origin="lower", extent=extent_lower)

plt.show()

ImportanceOfBeingErnest · 2018-04-08T01:40:23Z

lib/matplotlib/axes/_axes.py

-            corner of the axes. If None, default to rc `image.origin`.
+            corner of the axes. The convention 'upper' is typically used for
+            matrices; 'lower' is typically used for image data. Defaults to
+            :rc:`image.origin`.


I would say images are matrices. At least the convention is that images have the first pixels in the upper left corner.

Maybe better

The convention 'upper' is typically used for
matrices and images; 'lower' is typically used for displaying data arrays.

What are data arrays? numpy also uses the 'upper' convention:

>>> print(np.array([[1, 2], [3, 4]])) [[1, 2], [3, 4]]

For now, I don't have a good example for 'lower', so I just say

The convention 'upper' is typically used for matrices and images.

and don't give an example for lower.

timhoffm · 2018-04-08T09:16:30Z

Description for extent corrected.

ImportanceOfBeingErnest · 2018-04-08T17:13:36Z

By default, the center of the lower left
pixel is at (0, 0) and pixels have a size of (1, 1), i.e. extent
would be (-0.5, columns-0.5, -0.5, rows-0.5).

This is still not correct. The default origin is "upper" via the rcParams. Hence the lower left pixel is at (0,rows) (see left-most plot above). To achieve this plot by setting an extent you need [-0.5,a.shape[1]-0.5,a.shape[0]-0.5,-0.5] (as seen in the second plot).

In case the origin is "lower", the sentence is correct. (c.f. the right two plots above).

To be on the safe side here, maybe the only option is to clearly state both possible cases.

By default, pixels have a size of (1, 1).
Note that the extent is dependent on the origin setting.
If origin is set to upper, the center of the lower left
pixel is at (0, rows) , this corresponds to an extent
of (-0.5, columns-0.5, rows-0.5, -0.5).
If origin is set to "lower", the center of the lower left
pixel is at (0, 0). This is equivalent to an extent
of (-0.5, columns-0.5, -0.5, rows-0.5).

I guess ideally there should be a link to an example showing all possible cases.

phobson · 2018-04-08T18:49:05Z

I'm inclined to avoid the word "matrix" altogether since the numpy.matrix class is such a confusing mess and I'd like to avoid steering people in that direction. I propose to replace "matrix" with "2D-array". The term "raster" is all likely a appropriate, but I associate it (rightly or not) with GIS data.

timhoffm · 2018-04-08T18:58:05Z

@ImportanceOfBeingErnest hopefully extent is now more understandable. Also with a motivation, why this is a bit tricky.

An example would be great, but that's beyond this PR.

timhoffm · 2018-04-09T15:02:12Z

There's already a PR for an example for origin/extent: #10947

ImportanceOfBeingErnest · 2018-04-09T15:18:23Z

Can this be coincidence? That's perfect! I didn't see that at all. It definitely needs to be linked to (in both, origin and extent).

tacaswell · 2018-04-09T17:44:34Z

lib/matplotlib/axes/_axes.py

+            when interpolation is one of: 'sinc', 'lanczos' or 'blackman'.
+
+        resample
+            Undocumented.


can we sort out what resample does?

I couldn't. I can track down its use to _image_interpolation.h l.987. but that C code doesn't speak to me. It appears to be written by @mdboom. Maybe he could enlighten us?

It is documented in the resample C wrapper.

Thanks. I've copied the doc. Still I'm not 100% clear what this does:

When will resampling (no matter if full or partial) occur.

what are the output and the input images?

tacaswell · 2018-04-09T17:55:37Z

lib/matplotlib/axes/_axes.py

+            integer coordinates and their center coordinates range from 0 to
+            columns-1 horizontally and from 0 to rows-1 vertically.
+
+            Note that the meaning of *top* and *bottom* depend on *origin*


This is not quite right, I would say that 'origin' controls how the image is filled in, but not the meaning of 'top' and 'bottom'.

Changed to:

Note that the relation of top and bottom to data coordinates depends on origin.

"controls how the image is filled in" is true but does not help for describing the default values.

tacaswell

Still not happy with the changes to the extent docstring. Sorry for being difficult on this.

tacaswell · 2018-04-10T03:12:28Z

lib/matplotlib/axes/_axes.py

+            integer coordinates and their center coordinates range from 0 to
+            columns-1 horizontally and from 0 to rows-1 vertically.
+
+            Note that the relation of *top* and *bottom* to data coordinates


The meaning of 'top' and 'bottom' in data coordinates does not change, what changes is the orientation of the array in the bounding box.

https://8788-1385122-gh.circle-artifacts.com/0/home/circleci/project/doc/build/html/tutorials/intermediate/imshow_extent.html#sphx-glr-tutorials-intermediate-imshow-extent-py

The bounding box edges are exactly where expected in dataspace.

tacaswell · 2018-04-10T03:18:12Z

lib/matplotlib/axes/_axes.py

+            vertical axis.
+
+            - If origin is set to "upper", the center of the lower left
+              pixel is at (0, rows) , this corresponds to an extent


I suggest something like:

If origin is set to "upper" then the default extents is (...)

If origin is set to "lower" then the default extents is (...)

If the axes auto-scales the limits than this arranges such that the value in X[0, 0] is at (0, 0) in data-coordinates in both cases. In the case of "upper" the (0, 0) is in the upper left and the y-axis increases downward. In the case of "lower", the (0, 0) is in the lower left and the y-axis increases upward.

Maybe this needs a comment "having 'bottom > top' or 'left > right' may invert the drawn image"?

tacaswell · 2018-04-10T03:25:42Z

lib/matplotlib/axes/_axes.py

            The location, in data-coordinates, of the lower-left and
-            upper-right corners. If `None`, the image is positioned such that
-            the pixel centers fall on zero-based (row, column) indices.
+            upper-right corners.


'... of the image's bounding box'

timhoffm · 2018-04-10T07:19:31Z

No worries, it‘s this API being difficult, not you.

Maybe we‘re just using the wrong terminology. I would associate top and bottom with the visual appearance. For top > bottom I expect it to be above when I look at the figure. Probably we should talk about the min/max data coordinates, e.g. (xmin, xmax, ymin, ymax). Still have to think this through completely for all cases.

timhoffm · 2018-04-16T17:44:41Z

There's now only a basic mention of the interaction between extent and origin. This is too complicated to be described here in detail. Instead, both parameters should add a link to the example from #10947 once it gets merged.

ImportanceOfBeingErnest · 2018-04-26T13:16:04Z

This PR will also fix #10323 once it's merged.

ImportanceOfBeingErnest · 2018-04-27T08:07:27Z

@timhoffm can you recheck if this should use M x N or N x M (due to comment by @efiring)? Potentially #10225 would need to be adjusted as well.

timhoffm · 2018-04-28T09:05:36Z

Changed to more common notation (M, N).

timhoffm · 2018-05-02T21:15:31Z

@tacaswell Now that #10947 is merged and linked, I hope that the documentation on extent is right and this PR is good to go.

jklymak · 2018-05-06T16:34:36Z

lib/matplotlib/axes/_axes.py

-            axes.
+        aspect : {'equal', 'auto'} or float, optional
+            Controls the aspect ratio of the axes. The aspect is of particular
+            relevance for images since it may distort the image, i.e. pixel


i.e. pixels will not be square....

jklymak · 2018-05-06T16:34:59Z

lib/matplotlib/axes/_axes.py

-            If 'equal', and `extent` is None, changes the axes aspect ratio to
-            match that of the image. If `extent` is not `None`, the axes
-            aspect ratio is changed to match that of the extent.
+            This parameter is just a shortcut for explicitly calling


strike "just"....

jklymak · 2018-05-06T16:37:43Z

lib/matplotlib/axes/_axes.py

-            `X` must be an array of integers that index directly into
-            the lookup table of the `cmap`.
+        norm : `~matplotlib.colors.Normalize`, optional
+            If scalar data is used, the Normalize instance scales the


"... scalar data are used" (data is usually plural)....

jklymak · 2018-05-06T16:40:18Z

lib/matplotlib/axes/_axes.py

-            corner of the axes. If None, default to rc `image.origin`.
+            corner of the axes. The convention 'upper' is typically used for
+            matrices and images.
+            Defaults to :rc:`image.origin`.


I have no idea what image.origin defaults to. I think we could/should say what the default of the default is.

jklymak · 2018-05-06T16:49:24Z

lib/matplotlib/axes/_axes.py

-            The location, in data-coordinates, of the lower-left and
-            upper-right corners. If `None`, the image is positioned such that
-            the pixel centers fall on zero-based (row, column) indices.
+            By default, pixels have a size of (1, 1). They are centered on


I found this first sentence strange.

"""By default the extent of the image is one data unit per pixel, centered on integer co-ordinates. So the horizontal axis ranges from 0 to N-1, and the vertical axes from 0 to M-1. Specifying extent displays a subset of the data in this co-ordinate system.

By default the extent of the image is one data unit per pixel, centered on integer co-ordinates.

To me, that sounds even more strange. Extent is a range, which cannot be "one data unit per pixel". Also the extent is not centered, the pixels are.

Specifying extent displays a subset of the data in this co-ordinate system.

That sounds like clipping, which is not true. Extent scales the data.

Oops my mistake. What a confusing name for the kwarg. Regardless it still isn’t very clear.

How about pixels columns are linearly mapped between extent[0] and extent[3] and rows between ... by default these extents are ....

Wouldn't bother with columns and linear mapping to explain streching. Here's my current committed version:

The bounding box in data coordinates that the image will fill.
The image is stretched individually along x and y to fill the box.

The default extent is determined by the following conditions.
Pixels have unit size in data coordinates. Their centers are on
integer coordinates, and their center coordinates range from 0 to
columns-1 horizontally and from 0 to rows-1 vertically.

jklymak · 2018-05-06T19:42:27Z

I'll give @tacaswell another day to comment if he wants, and then we can merge. Its a big improvement, and can be refined again if necessary.

ImportanceOfBeingErnest · 2018-05-07T10:17:17Z

lib/matplotlib/axes/_axes.py

-            See also the `filternorm` and `filterrad` parameters.
-            If `interpolation` is 'none', then no interpolation is performed
+            See also the *filternorm* and *filterrad* parameters.
+            If *interpolation* is 'none', then no interpolation is performed
            on the Agg, ps and pdf backends. Other backends will fall back to
            'nearest'.



A totally minor suggestion: At this point it could make sense to link to the interpolation_methods example.

Sure. Added. Revisiting the interpolation docstring revealed that it was not good anyway. So improved that as well.

stale review

jklymak · 2018-05-07T21:58:49Z

Thanks a lot @timhoffm - if anyone still has issues w/ this, we can have a new PR 😉....

timhoffm added the Documentation label Apr 8, 2018

ImportanceOfBeingErnest reviewed Apr 8, 2018

View reviewed changes

timhoffm force-pushed the doc-axes-imshow branch from beed2b6 to dbd28f6 Compare April 8, 2018 09:14

timhoffm force-pushed the doc-axes-imshow branch from dbd28f6 to 28ade6a Compare April 8, 2018 09:21

timhoffm force-pushed the doc-axes-imshow branch from 28ade6a to b0c5c56 Compare April 8, 2018 18:55

ImportanceOfBeingErnest mentioned this pull request Apr 9, 2018

DOC: add tutorial explaining imshow *origin* and *extent* #10947

Merged

tacaswell reviewed Apr 9, 2018

View reviewed changes

tacaswell added this to the v3.0 milestone Apr 9, 2018

timhoffm force-pushed the doc-axes-imshow branch 2 times, most recently from f05bea3 to 3f985bd Compare April 9, 2018 23:06

tacaswell previously requested changes Apr 10, 2018

View reviewed changes

timhoffm force-pushed the doc-axes-imshow branch 2 times, most recently from 7b85456 to 801226d Compare April 16, 2018 16:40

timhoffm force-pushed the doc-axes-imshow branch 2 times, most recently from db056fd to feeb1a3 Compare April 16, 2018 22:07

ImportanceOfBeingErnest mentioned this pull request Apr 25, 2018

Clarify array shape vs image size in imshow() docstring #11125

Closed

6 tasks

timhoffm force-pushed the doc-axes-imshow branch from feeb1a3 to 1e12880 Compare April 28, 2018 09:04

timhoffm force-pushed the doc-axes-imshow branch from 1e12880 to 7c958a8 Compare May 2, 2018 21:14

timhoffm added the status: needs review label May 5, 2018

jklymak reviewed May 6, 2018

View reviewed changes

timhoffm force-pushed the doc-axes-imshow branch from 7c958a8 to 3ece71c Compare May 6, 2018 19:29

jklymak approved these changes May 6, 2018

View reviewed changes

ImportanceOfBeingErnest reviewed May 7, 2018

View reviewed changes

Improve docstring of Axes.imshow

f308ccc

timhoffm force-pushed the doc-axes-imshow branch from 3ece71c to f308ccc Compare May 7, 2018 20:19

jklymak merged commit 9c7e0de into matplotlib:master May 7, 2018

timhoffm deleted the doc-axes-imshow branch May 7, 2018 22:02

ImportanceOfBeingErnest mentioned this pull request May 22, 2018

imshow documentation has confusing matrix dimensions #10323

Closed

QuLogic removed the status: needs review label Jul 5, 2018

Improve docstring of Axes.imshow #10982

Improve docstring of Axes.imshow #10982

Conversation

timhoffm commented Apr 8, 2018 • edited Loading

PR Summary

anntzer commented Apr 8, 2018

ImportanceOfBeingErnest commented Apr 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm commented Apr 8, 2018

ImportanceOfBeingErnest commented Apr 8, 2018

phobson commented Apr 8, 2018

timhoffm commented Apr 8, 2018

timhoffm commented Apr 9, 2018 • edited Loading

ImportanceOfBeingErnest commented Apr 9, 2018

Choose a reason for hiding this comment

timhoffm Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm commented Apr 10, 2018 • edited Loading

timhoffm commented Apr 16, 2018 • edited Loading

ImportanceOfBeingErnest commented Apr 26, 2018

ImportanceOfBeingErnest commented Apr 27, 2018

timhoffm commented Apr 28, 2018

timhoffm commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak May 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented May 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented May 7, 2018

timhoffm commented Apr 8, 2018 •

edited

Loading

timhoffm commented Apr 9, 2018 •

edited

Loading

timhoffm Apr 9, 2018 •

edited

Loading

timhoffm commented Apr 10, 2018 •

edited

Loading

timhoffm commented Apr 16, 2018 •

edited

Loading

jklymak May 6, 2018 •

edited

Loading