iframe-proxy

jklymak · 2018-05-10T04:22:48Z

PR Summary

The transforms tutorial mentions fig.dpi_scale_trans but it really seems it should be up in the list of basic transforms. Added a couple of examples as well...

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

ImportanceOfBeingErnest · 2018-05-10T17:33:31Z

+        ax.transData.transform((xdata[0], ydata[0])))
+# plot a circle around the point that is 50 points in diameter...
+circle = mpatches.Circle((x, y), 50/72, fill=None,
+        transform=fig.dpi_scale_trans)


Not sure if this is a good example. If you want a circle with 50 point radius you'd just plot a plot with circular marker

ax.plot(xdata, ydata, ls="", marker="o", ms=100, mec="k", mfc="none")

Sure fair enough. The point wasn't to do that particular thing, but just to show the use of inverted, which the tutorial barely touched on.

I think it would be good to explain inverted() somewhere.

In the example here inverted() is not well explained. It's just there. Second, the example reads like this would be the way to go for placing some artist at a point in data coordinates. I doubt that is how one should do it. Essentially because of the drawback that it does not shift together with the data points.

To place some general path, like the ellipse here one could use

ell = mpatches.Ellipse((0, 0), 150/72., 130/72., angle=40) p = ell.get_path() verts = ell.get_patch_transform().transform(p.vertices) codes = p.codes p = mpath.Path(verts, codes) ax.scatter(xdata, ydata, marker=p, s = (2*72)**2)

Fair enough - inverted was explained below, but probably needs to be explained here.

Your suggested change is ... inelegant.... There must be a better way?

Ahhh:

mytrans = fig.dpi_scale_trans + mtransforms.ScaledTranslation(xdata[0], ydata[0], ax.transData) print(mytrans) ell = mpatches.Ellipse((0, 0), 150/72., 130/72., angle=40, transform=mytrans) ax.add_patch(ell)

That's a very good example. The problem is that ScaledTranslation and transform chaining are discussed later. So one should point/link to that in the text or maybe put the whole section more downwards?

I modified the description below this to account for the changes here...

jklymak · 2018-05-10T18:13:29Z

https://9697-1385122-gh.circle-artifacts.com/0/home/circleci/project/doc/build/html/tutorials/advanced/transforms_tutorial.html#sphx-glr-tutorials-advanced-transforms-tutorial-py

Not sure about the small figures in the one block; they are fine on my screen but maybe too small on some?

ImportanceOfBeingErnest · 2018-05-10T19:11:29Z

+circ = mpatches.Circle((2.5, 2), 1.0, transform=fig.dpi_scale_trans,
+                      facecolor='yellow', alpha=0.5)
+ax.add_patch(circ)
+plt.show()


Could it make sense to get rid of that strong green and pale yellow? After all it's the circle that is important here, not the 1000 scatter points.

ImportanceOfBeingErnest · 2018-05-10T20:10:25Z

To give you an idea of how the small figures look on my screen:

It's probably a matter of taste.

jklymak · 2018-05-10T20:12:35Z

No thats too small. Changed it...

ImportanceOfBeingErnest · 2018-05-10T20:51:18Z

+# first be transfromed to ``display`` coordinates (``[ 358.4  475.2]`` on
+# a 200-dpi monitor) and then those coordinates
+# would be scaled by ``fig.dpi_scale_trans`` pushing the center of
+# the ellipse well off the screen (i.e. ``[ 71680.  95040.]``).


Mhh, why do you explain all the things that would go wrong if you did something other than you're actually doing? Instead why not simply explain what you are doing and giving the coordinates for the actual case?
I would also think this is better done after the example code.

jklymak · 2018-05-10T22:20:28Z

It would still be good to have an inverted example in here somewhere that was useful. Suggestions?

ImportanceOfBeingErnest · 2018-05-20T09:50:27Z

+for artists placed in an axes or figure to have their transform set to
+something *other* than the ``IdentityTransform()``; the default when an artist
+is placed on an axes using ``~Axes.axes.add_artist`` is for the transform to be
+`ax.transData`.


Some mixup between single and double quotes here?

`~.transforms.IdentityTransform()`

`~.Axes.axes.add_artist`

``ax.transData``

This should mostly be single '`' I think (so they link).

ImportanceOfBeingErnest · 2018-05-20T09:55:24Z

+plt.show()
+
+###############################################################################
+# If we change the figure size...


I don't like ellipses (...). Either one states what this is meant to be left out or not. In this case, it's probably useful to descibe what happens if you change the figure size, e.g.

"If you change the figure size, the circle does not change its absolute position is hence cropped."

or similar.

ImportanceOfBeingErnest · 2018-05-20T11:38:23Z

There is already some example for inverted using simple coordinates towards the top of the page.

Thinking about it for some time I do not have a good idea for another inverted example. The problem with inverted is that it freezes the transform. So in most cases you try to avoid using it. And in cases you need to use it, it gets a little complicated because you would need to add a callback to garantee interactivity.

Examples where I used inverted on Stackoverflow:

They all seem to be too complex for a simple example in a tutorial.

ImportanceOfBeingErnest

Modulo the double backticks in the second paragraph, I would approve this. This is a significant improvement compared to the previous version of the tutorial and adds a really useful example for using inches as figure coordinates.
I would not postpone this just because no other inverted example can be found. In the limit this can be another PR if it's really desired.

jklymak · 2018-05-28T20:46:09Z

This got a lot of commentary, I think it can be merged....

timhoffm

Overall great.

Some minor style issuse: The ones mentioned and additionally:

Some indents for line continuations are weird
Could use fig, ax = plt.subplots throughout.

timhoffm · 2018-05-29T00:12:33Z

        autoscaling.

+        If no ``transform`` has been specified when creating the artist (e.g.
+        ``artist.get_transform()==None``) then the transform is set to


spaces around ==

timhoffm · 2018-05-29T00:14:51Z

+ax.set_xlim((0, 1))
+
+trans =  fig.dpi_scale_trans + \
+        transforms.ScaledTranslation(xdata[0], ydata[0], ax.transData)


strange indent/spacing

Oh, hmmm. I double indent for line continuations. Is that strange? I thought it helped make it clear that its not a new code block.

Can you use () continuation instead of \?

OK< change indent too.

timhoffm · 2018-05-29T00:15:12Z

+
+# plot an ellipse around the point that is 150 x 130 points in diameter...
+circle = mpatches.Ellipse((0, 0), 150/72, 130/72, angle=40, fill=None,
+        transform=trans)


strange indent/spacing

... as above...

These typically get moved up to align with the opening ( that it is in.

tacaswell · 2018-05-29T01:51:00Z

+# and then translate the data to `xdata[0]` and `ydata[0]` in data space.
+#
+# In interactive use, the ellipse stays the same size even if the
+# axes limits are changed via zoom, but the ellipse's size doesn't change.


Repetitive sentence.

timhoffm

Please tell me if I get too picky about the formatting. These things just jump to my eye. So I mention them. They are not essential for a merge if it gets too much.

timhoffm · 2018-05-29T06:29:27Z

+ax.plot(xdata, ydata, "o")
+ax.set_xlim((0, 1))
+
+trans =  (fig.dpi_scale_trans +


extra space before =

...we love you because these things jump out at you...

timhoffm · 2018-05-29T06:31:40Z

-circ = patches.Circle((0.5, 0.5), 0.25, transform=ax.transAxes,
-                      facecolor='yellow', alpha=0.5)
+circ = mpatches.Circle((0.5, 0.5), 0.25, transform=ax.transAxes,
+                      facecolor='blue', alpha=0.75)


the continued line should start after the enclosing bracket:

circ = mpatches.Circle((0.5, 0.5), 0.25, transform=ax.transAxes, facecolor='blue', alpha=0.75)

This happens multiple times in the example (not only your edits).

timhoffm · 2018-05-29T21:04:37Z

-+-----------+-----------------------------+-----------------------------------+
+----------------+-----------------------------+-----------------------------------+
+|Coordinates     |Transformation object        |Description                        |
+----------------+-----------------------------+-----------------------------------+


Hint: You can make the texts above headers by changing all - to = in this line.

afvincent

A few typos noticed during a quick reading of the text.

afvincent · 2018-05-29T21:22:26Z

+plt.show()
+
+###############################################################################
+# If we change the figure size the circle does not change its absolute


" (...) the circle does not change its absolute position is cropped." Would it not be missing some punctuation at the end of the sentence? Not sure that I understand what is cropped.

afvincent · 2018-05-29T21:22:50Z

+plt.show()
+
+###############################################################################
+# Another use is putting a patch with a set physical dimmension around a


afvincent · 2018-05-29T21:26:51Z

+#
+# Note the use of the plus operator on the transforms below.
+# This code says: first apply the scale transformation ``fig.dpi_scale_trans``
+# to make the Ellipse the proper size, but still centered at (0, 0),


Is the capital E in "Ellipse" used on purpose?

afvincent · 2018-05-29T21:30:43Z

+#   in data space to the correct spot.
+#   If we had done the ``ScaledTranslation`` first, then
+#   ``xdata[0]`` and ``ydata[0]`` would
+#   first be transfromed to ``display`` coordinates (``[ 358.4  475.2]`` on


transformed

jklymak · 2018-05-30T18:23:18Z

@afvincent thanks for ctaching the typos... Someone want to merge this?

lumberbot-app · 2018-06-05T19:20:42Z

jklymak added the Documentation label May 10, 2018

jklymak changed the title ~~DOC: add infor to transforms tutorial about fig.dpi_scale_trans~~ DOC: add to transforms tutorial re fig.dpi_scale_trans May 10, 2018

jklymak closed this May 10, 2018

jklymak force-pushed the doc-transform-tut-dpi branch from fae876c to 4d1ca36 Compare May 10, 2018 04:40

jklymak reopened this May 10, 2018

jklymak force-pushed the doc-transform-tut-dpi branch 2 times, most recently from c19783e to 9f0e5a7 Compare May 10, 2018 17:14

jklymak added the status: work in progress label May 10, 2018

ImportanceOfBeingErnest reviewed May 10, 2018

View reviewed changes

jklymak force-pushed the doc-transform-tut-dpi branch from 9f0e5a7 to 23797b9 Compare May 10, 2018 17:46

jklymak removed the status: work in progress label May 10, 2018

ImportanceOfBeingErnest reviewed May 10, 2018

View reviewed changes

jklymak force-pushed the doc-transform-tut-dpi branch 2 times, most recently from 744edfa to 3b093dc Compare May 10, 2018 20:05

jklymak force-pushed the doc-transform-tut-dpi branch from 3b093dc to e24619d Compare May 10, 2018 20:12

jklymak force-pushed the doc-transform-tut-dpi branch from e24619d to 3127333 Compare May 10, 2018 20:48

ImportanceOfBeingErnest reviewed May 10, 2018

View reviewed changes

jklymak force-pushed the doc-transform-tut-dpi branch from 3127333 to 22a3703 Compare May 10, 2018 21:35

jklymak added this to the v2.2-doc milestone May 14, 2018

ImportanceOfBeingErnest reviewed May 20, 2018

View reviewed changes

ImportanceOfBeingErnest approved these changes May 20, 2018

View reviewed changes

timhoffm approved these changes May 29, 2018

View reviewed changes

tacaswell reviewed May 29, 2018

View reviewed changes

jklymak force-pushed the doc-transform-tut-dpi branch from 22a3703 to 0059f22 Compare May 29, 2018 04:27

timhoffm reviewed May 29, 2018

View reviewed changes

jklymak force-pushed the doc-transform-tut-dpi branch from 0059f22 to 35e3055 Compare May 29, 2018 13:51

timhoffm reviewed May 29, 2018

View reviewed changes

afvincent reviewed May 29, 2018

View reviewed changes

DOC: add info to transforms tutorial about fig.dpi_scale_tran

b81ca50

jklymak force-pushed the doc-transform-tut-dpi branch from 35e3055 to b81ca50 Compare May 30, 2018 00:36

phobson merged commit bc0882e into matplotlib:master Jun 5, 2018

lumberbot-app Bot added the Still Needs Manual Backport label Jun 5, 2018

dstansby removed the Still Needs Manual Backport label May 25, 2019

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Uh oh!

Conversation

jklymak commented May 10, 2018

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ImportanceOfBeingErnest commented May 10, 2018

Uh oh!

jklymak commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ImportanceOfBeingErnest commented May 20, 2018

Uh oh!

ImportanceOfBeingErnest left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commented May 28, 2018

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm May 29, 2018 •

edited

Loading