Add a dpi transform to AuxTransformBox by has2k1 · Pull Request #7344 · matplotlib/matplotlib

has2k1 · 2016-10-25T10:28:55Z

Issue
While the children themselves (e.g. Text objects) of an AuxTransformBox
do conform to the dpi of the figure, the locations of the objects
do not. Therefore the space taken up by the AuxTransformBox remains
just about fixed, regardless of dpi. The relative distance between
any two objects in the AuxTransformBox remains absolutely fixed. This
leads to "scaling" artefacts for png output when the dpi is not 72.

Solution
Add a similar dpi transformation to that done for the DrawingArea.

has2k1 · 2016-10-26T13:50:50Z

Please wait on considering this PR.

tacaswell · 2016-10-27T20:31:57Z

That looks like it fixes a pretty major bug!

dopplershift

Just a minor pep8 nit. Otherwise, I think this looks good.

dopplershift · 2016-10-27T21:20:52Z


-        return ub.width, ub.height, 0., 0.
+        dpi_cor = renderer.points_to_pixels(1.)
+        return ub.width*dpi_cor, ub.height*dpi_cor, 0., 0.


Minor nit: need spaces around * for pep8.

I think pep8 is fine with that for arithmetic operators, it calls for consistency, readability/good judgment. Either way, when I make a revision to the PR that line will not be there(for a yet unknown reason).

pep8 is fine with that. Both the document and the tool.

efiring · 2016-10-27T23:33:45Z

On 2016/10/27 1:17 PM, Hassan Kibirige wrote:

I think pep8 is fine with that for arithmetic operators, it calls for
consistency, readability/good judgment.

The actual PEP 8 does, exactly as you say--but the automated checker
does not. Contrary to the letter and the spirit of PEP 8, the automated
checking regime is a strict application of rules. No judgment is
allowed. It has its advantages in yielding consistency, but it can be
annoying.

(Sorry, I couldn't resist the digression.)

has2k1 · 2016-10-27T23:50:01Z

@efiring, thanks for the digression I prefer the automated solution and you have put question marks around the efficacy of the tools I use. According the .travis.yml the automated checker is the pep8 package, the same thing I use for my editor. Also the manual invocation of which

pep8 lib/matplotlib/offsetbox.py | grep 970

yields nothing.

dopplershift · 2016-10-28T15:01:20Z

Because of course the automatic tool and the document can't be in agreement. On the plus side, I learned more about the actual document today.

NelleV · 2016-10-28T20:39:38Z

@dopplershift The tool and the document are on agreement on that particular point.

dopplershift · 2016-10-28T20:42:16Z

I swear I've seen it error on that--apparently I'm wrong.

NelleV · 2016-10-28T20:43:23Z

You are not wrong. Just outdated :-D
That particular point was fixed in a release of pep8 late 2012/early 2013

efiring · 2016-10-28T20:47:22Z

That's good to know--it's a big improvement. I think that (a*b + c*d) is often more readable than (a * b + c * d).

NelleV · 2016-10-28T20:50:09Z

If we want to be more reliable on enforcing pep8 on new patches, we could run flake8 on the diff of the patch instead of how we currently run pep8 (which only looks at whether the number of pep8 violations is lower or equal than the previous patch). Some projects do that (joblib, scikit-learn and maybe ipython/jupyter folks). It should be quite easy to set up.

NelleV · 2016-10-28T20:50:48Z

@has2k1 Do you mind explaining why we should wait on considering this PR?

Apparently I don't understand pep8. 😁

has2k1 · 2016-10-28T22:29:15Z

@NelleV, The solution so far does not completely solve the problem, as straight forward as it may be! A slight (and not good looking) modification though yields better results but it is not stable. I will put up a more complete illustration that should aid in getting better help.

@has2k1

Because @has2k1 has a better fix in the pipeline :D

NelleV · 2016-11-08T23:03:47Z

Hi @has2k1
I know you have a better fix in the pipeline, but do you mind if we merge this in the mean time? The current patch does fix a major bug in matplotlib, and I think that having an approximate fix is much better than no fix at all.
Thanks,
N

has2k1 · 2016-11-08T23:19:43Z

@NelleV, hold on for a day. I'll push update and a dump of my worksheet that shows the limitations.

**Issue** While the children themselves (e.g. Text objects) to an AuxTransformBox do conform to the dpi of the figure, the locations of the objects do not. Therefore the space taken up by the AuxTransformBox remains just about fixed, regardless of dpi. The relative distance between any two objects in the AuxTransformBox remains absolutely fixed. This leads to "scaling" artefacts for *png* output when the dpi is not 72. **Solution** Add a similar dpi transformation to that done for the DrawingArea.

QuLogic · 2016-11-13T23:06:42Z

Try enabling the frame on the AnchoredSizeBar and see if you can figure out where it really is now.

has2k1 · 2016-11-13T23:16:11Z

I did that. It is at the bottom. Some of it below the x-axis.

has2k1 · 2016-11-17T04:00:37Z

My previous response was facilitated by a poor memory. This is the result.

The problem is that for the AnchoredSizeBar the AuxTransformBox is created with transform=ax.transData. If you used an IdentityTransform as is the case for the red boxes, then it works.

efiring · 2016-11-25T22:04:45Z

Is this ready to go after a rebase? Or are there still problems with it? If so, is it really release-critical?

QuLogic · 2016-11-26T21:34:56Z

I don't think this is one rebase away from being ready; there's still the question of what to do with AnchoredSizeBar. I've convinced myself that it is working correctly, so something will have to be done to ensure it works the same way once this PR is okay.

You'll have to ask @NelleV why it's release critical.

has2k1 · 2016-11-27T20:59:54Z

I am in agreement with @QuLogic. I have without success tried to find a fix for AnchoredsizeBar.

NelleV · 2016-11-28T03:52:42Z

We can remove the release critical label, but this is a major bug. All the publications that use this are wrong, and we are probably talking thousands of publications.

QuLogic · 2016-11-28T05:06:42Z

I'm not so sure it's that major. AuxTransformBox is not used anywhere in matplotlib proper. It is used in axes_grid1.anchored_artists.{AnchoredAuxTransformBox,AnchoredEllipse,AnchoredSizeBar} and provided as an alias in axes_grid1.anchored_artists. In the former case, the documentation suggests using something like ax.transData as the transform, which includes DPI automatically. AuxTransformBox itself is not really well documented.

This bug only arises when using AuxTransformBox with IdentityTransform, which by definition does not include a DPI adjustment, and it only affects text because fonts are always specified in points which do obey DPI. So is it really a bug, or is just an unfortunate combination of many factors? Are there actually a whole lot of publications using AuxTransformBox in this manner, or do they use one of the wrappers, which seem to work fine if used as documented?

QuLogic · 2016-12-07T03:37:17Z

I'm un-critical-izing this as I don't think it will be fixed soon and I'm not so sure it's that critical.

When the dpi is not 72, the legend text of the `colorbar` does not align with the ticks. The patch works for our case but has other complications, as a result it is not merged in Matplotlib. See: matplotlib/matplotlib#7344

jklymak · 2020-09-12T19:46:50Z

I'm pretty confused if this really needs to be fixed or just closed. Hopefully one of the original proponents can chime in again? Otherwise I've marked stale and will close in a month or so.

has2k1 · 2020-09-12T21:14:43Z

As I recall, it is not easy fix to this issue without breaking AnchoredSizeBar which relies on the "bug". If we should fix it, we can create a special (internal) subclass of AuxTransformBox that is used by AnchoredSizeBar so that it is not broken by the solution!

For my use case in Plotnine, I created a AuxTransformBox subclass that worked as I expected.

jklymak · 2020-09-12T21:23:47Z

I don't at all mind this going forward... I've just moved to draft, and feel free to update or start again if you are interested. I'm just cleaning up and haven't thought about it deeply.

jklymak · 2021-06-14T18:56:22Z

I'll close as abandoned here.

has2k1 force-pushed the fix-auxtransformbox-dpi branch 3 times, most recently from 136f895 to ddf6d95 Compare October 26, 2016 10:59

tacaswell added this to the 2.0.1 (next bug fix release) milestone Oct 27, 2016

dopplershift previously requested changes Oct 27, 2016

View reviewed changes

NelleV changed the title ~~Add a dpi transform to AuxTransformBox~~ [MRG+1] Add a dpi transform to AuxTransformBox Oct 28, 2016

NelleV added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 28, 2016

NelleV previously approved these changes Oct 28, 2016

View reviewed changes

has2k1 mentioned this pull request Oct 29, 2016

PEP8: making travis run pep8 on diff instead of comparing the total number of violation to be more robust. #7358

Closed

NelleV changed the title ~~[MRG+1] Add a dpi transform to AuxTransformBox~~ Add a dpi transform to AuxTransformBox Oct 29, 2016

NelleV modified the milestones: 2.0 (style change major release), 2.0.1 (next bug fix release) Oct 31, 2016

NelleV mentioned this pull request Nov 9, 2016

MAINT flake8 is now run on diff + travis cleanup. #7403

Closed

1 task done

QuLogic removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Dec 7, 2016

QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0 (style change major release) Dec 7, 2016

tacaswell mentioned this pull request Dec 21, 2016

anchored_artists don't support zorder argument #7650

Closed

2 tasks done

dstansby added the status: needs revision label Feb 8, 2017

QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0.2 (next bug fix release) May 3, 2017

tacaswell modified the milestones: 2.1.1 (next bug fix release), 2.2 (next feature release) Oct 9, 2017

jklymak added status: orphaned PR stale labels Sep 12, 2020

jklymak marked this pull request as draft September 12, 2020 21:22

jklymak closed this Jun 14, 2021

story645 removed this from the future releases milestone Oct 6, 2022

Conversation

has2k1 commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

has2k1 commented Oct 26, 2016

Uh oh!

tacaswell commented Oct 27, 2016

Uh oh!

dopplershift left a comment

Choose a reason for hiding this comment

Uh oh!

dopplershift Oct 27, 2016

Choose a reason for hiding this comment

Uh oh!

has2k1 Oct 27, 2016 • 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!

NelleV Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

efiring commented Oct 27, 2016

Uh oh!

has2k1 commented Oct 27, 2016

Uh oh!

dopplershift commented Oct 28, 2016

Uh oh!

NelleV commented Oct 28, 2016

Uh oh!

dopplershift commented Oct 28, 2016

Uh oh!

NelleV commented Oct 28, 2016

Uh oh!

efiring commented Oct 28, 2016

Uh oh!

NelleV commented Oct 28, 2016

Uh oh!

NelleV commented Oct 28, 2016

Uh oh!

has2k1 commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NelleV commented Nov 8, 2016

Uh oh!

has2k1 commented Nov 8, 2016

Uh oh!

QuLogic commented Nov 13, 2016

Uh oh!

has2k1 commented Nov 13, 2016

Uh oh!

has2k1 commented Nov 17, 2016

Uh oh!

efiring commented Nov 25, 2016

Uh oh!

QuLogic commented Nov 26, 2016

Uh oh!

has2k1 commented Nov 27, 2016

Uh oh!

NelleV commented Nov 28, 2016

Uh oh!

QuLogic commented Nov 28, 2016

Uh oh!

QuLogic commented Dec 7, 2016

Uh oh!

jklymak commented Sep 12, 2020

Uh oh!

has2k1 commented Sep 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jklymak commented Sep 12, 2020

Uh oh!

jklymak commented Jun 14, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

has2k1 commented Oct 25, 2016 •

edited

Loading

has2k1 Oct 27, 2016 •

edited

Loading

has2k1 commented Oct 28, 2016 •

edited

Loading

has2k1 commented Sep 12, 2020 •

edited

Loading