Repository navigation
Deprecate getting/setting pix on WCSAxes Spine - #13989
Conversation
|
Test failures look real. |
|
Hello!
Does this mean that we cannot deprecate? Or does it mean that the tests or some code need update? Do you need assistance? |
|
Needs an update, I should be able to look into this myself when I have time. |
|
👍 this should be good for review now, sorry for the delay! |
| self._world = None | ||
| else: | ||
| self._pixel = self.parent_axes.transData.transform(self._data) | ||
| self._data = value |
There was a problem hiding this comment.
I think this line can be removed. It's not a duplicate of line 149.
| self._data = value | ||
| if value is None: | ||
| self._pixel = None | ||
| self._data = None |
|
👍 done |
|
@larrybradley , please re-review. Thanks! |
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
Looks good to me! But could we include an image test for the bugfix, even if it needs updating once the remaining issues are resolved in follow-up PRs/matplotlib?
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
This looks good to me!
I went down a rabbit hole trying to set up a test for this because it does not happen with PNG output, but does with e.g. PDF. So I have been working on extending pytest-mpl to be able to compare PDF output (see matplotlib/pytest-mpl#194). So let's plan to add a regression test in future once the pytest-mpl changes are in a release.
astrofrog
added
the
zzz 💤 merge-when-ci-passes
|
(I have rebased this to make sure the test failures were unrelated) |
|
@larrybradley , do you want to re-review since you requested changes? |
larrybradley
left a comment
larrybradley
left a comment
There was a problem hiding this comment.
LGTM. I just pushed a small change to remove an unnecessary line.
pllim
added
the
API change
|
Thanks, all! |
4 participants
Description
(this is a revival of #12621)
When using Matplotlib, it is generally only safe to calculate pixel coordinates of artists at draw time, as that is when the figure DPI is set in stone. Calculating the pixel coordinates before drawing can lead to issues like #12568.
This PR fixes this issue for
WCSAxesSpines by ensuring that pixel calculations are done at draw time. It also deprecates getting and setting pixel coordinates.This pull request is to partly address #12568. Before that issue can be fully resolved I think there is also a fix required on the Matplotlib side.
With the sample code given in #12568, this improves the resulting plot from this (on current main):
to this:
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
Extra CIlabel.no-changelog-entry-neededlabel. If this is a manual backport, use theskip-changelog-checkslabel unless special changelog handling is necessary.astropy-botcheck might be missing; do not let the green checkmark fool you.backport-X.Y.xlabel(s) before merge.