Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Only clear Axis once when creating an Axes#26164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
a15b0ec to8107d05CompareThis breaks polar unfortunately, but maybe thats easy to fix? |
oscargus commentedJun 21, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Seems like registering the axis fixes it locally (except one test where the PDF doesn't pass, but all other formats...). I honestly cannot spot the difference when switching between the two PDFs, but apparently three pixels are different in the PNG version of the PDF... |
f07c0b0 to2aa5cddCompareThe next thing is probably to skip using But that will not go in this PR. |
| + self.transAxes) | ||
| # Must have a dummy spine for 'inner' | ||
| inner_spine = Spine.linear_spine(self, 'bottom') | ||
| return {'polar': polar_spine, 'inner': inner_spine} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is requiring this extra 'inner' something other axes are going to need? eg, maybe check this works w/ cartopy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, I'm not happy with this consequence.
I think a better solution is to splitSpline.clear into two, so that one can avoid clearing the registered axis. In this way there will only be one clear per axis and this can be avoided.
It still opens the question as to whether the spines should have their Axis registered (as that now seems possible without any test failures).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
FWIW I just ran the Cartopy tests against this locally and they seem fine to me.
== 1 failed, 790 passed, 24 skipped, 2 xfailed, 2 xpassed, 8585 warnings in 84.52s (0:01:24) ==(the one failure is related to#25162 I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks!
That probably means that we can still register the Axis on the Spines, but it would still be better to only clear the Axis once.
Now this only clears the Axis once. Thought it made sense to create a separate method for clearing the "spine-part" of the spine, despite only a single statement in it. Better readability and easier to expand. Now, the Axis is registered to the spine for polar and geo, although it is from clear to me what this spine dict actually does and how it is documented (and how that differs between Axes-classes). |
| @@ -2619,7 +2625,8 @@ def set_ticks_position(self, position): | |||
| self.set_tick_params(which='both', right=True, labelright=False, | |||
| left=True, labelleft=True) | |||
| else: | |||
| assert False, "unhandled parameter not caught by _check_in_list" | |||
| _api.check_in_list(['left', 'right', 'both', 'default', 'none'], | |||
| position=position) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This halves the number of calls tocheck_in_list, from ~100.000 to ~50.000 in the example. For cases like this, where it is called from a commonly used__init__-method it is probably better to have it last. Although, in general error checking may be better off early.
| @@ -209,7 +214,8 @@ def _apply_tickdir(self, tickdir): | |||
| # further updates ticklabel positions using the new pads. | |||
| if tickdir is None: | |||
| tickdir = mpl.rcParams[f'{self.__name__}.direction'] | |||
| _api.check_in_list(['in', 'out', 'inout'], tickdir=tickdir) | |||
| else: | |||
| _api.check_in_list(['in', 'out', 'inout'], tickdir=tickdir) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The rcParam is already validated.
| if m is None: | ||
| m = "default" | ||
| else: | ||
| _api.check_in_list(("anchor", "default"), rotation_mode=m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If this change is OK one should probably clarify that settingNone will return"default" in the doc-string.
| @@ -674,7 +678,12 @@ def __init__(self, axes, *, pickradius=15): | |||
| self._major_tick_kw = dict() | |||
| self._minor_tick_kw = dict() | |||
| self.clear() | |||
| if clear: | |||
| self.clear() | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess I thoughtclear did alot more than this.
I think that the doctoring should maybe clarify what the heck "clear on creation" means, or why one would or would not want to do it. I'm pretty surprised that everything "works" with just setting the convert and units to "None". If that is all robust (and tests seem to pass!) that is really a great simplification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The thing is thatAxis.clear is called fromAxes.clear, sp in that case we don't not have to call it on creation.
Those two attributes are defined by clear, but since they are checked before clear is called now, they must be defined in__init__.
I do not really think that calling clear as part of the creation is a very nice design pattern, but I guess it requires quite a bit of an effort to get rid of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Modulo fixing the doc string
I have updated the doc-string. I would just like to highlight another change here that may or may not be controversial. Passing |
| clear : bool, default: True | ||
| Whether to clear the Axis on creation. This is not required, e.g., when | ||
| creating an Axis as part of an Axes, as ``Axes.clear`` will call | ||
| ``Axis.clear``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not possible to link as there are threeAxes.clear methods and I wanted to clearly state the difference betweenAxes.clear andAxis.clear.
| from library code where it is known that the Axis is cleared separately. | ||
| """ | ||
| self._position = None # clear position | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| self.stale=True | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll try to do a follow up PR, I suspect most of theclear methods do not mark the artist as stale when they should....
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@oscargus please self merge if you are OK with@tacaswell's changes! |
Uh oh!
There was an error while loading.Please reload this page.
PR summary
Helps with#26162
Speed up the Axes creation. Reduces the number of calls to
Axis.clearfrom six to one per axis leading to about 20% reduction for a 10 x 10 subplot.Seems like this (or some other change) enables registering polar and geo axis to spines.
PR checklist