Only clear Axis once when creating an Axes #26164

oscargus · 2023-06-21T17:06:10Z

PR summary

Helps with #26162

Speed up the Axes creation. Reduces the number of calls to Axis.clear from 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

"closes #0000" is in the body of the PR description to link the related issue
new and changed code is tested
Plotting related features are demonstrated in an example
New Features and API Changes are noted with a directive and release note
Documentation complies with general and docstring guidelines

jklymak · 2023-06-21T18:46:24Z

This breaks polar unfortunately, but maybe thats easy to fix?

oscargus · 2023-06-21T19:04:22Z

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...

oscargus · 2023-06-21T20:24:13Z

The next thing is probably to skip using set for Tick and call the setters directly and/or set the attributes directly. Creating ticks is about 40% of the time right now.

But that will not go in this PR.

jklymak · 2023-06-22T03:31:21Z

galleries/examples/specialty_plots/radar_chart.py

+                                          + self.transAxes)
+                # Must have a dummy spine for 'inner'
+                inner_spine = Spine.linear_spine(self, 'bottom')
+                return {'polar': polar_spine, 'inner': inner_spine}


Is requiring this extra 'inner' something other axes are going to need? eg, maybe check this works w/ cartopy?

Yeah, I'm not happy with this consequence.

I think a better solution is to split Spline.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).

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)

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.

oscargus · 2023-06-22T11:54:54Z

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).

oscargus · 2023-06-22T13:26:20Z

lib/matplotlib/axis.py

@@ -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)


This halves the number of calls to check_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.

oscargus · 2023-06-22T13:27:14Z

lib/matplotlib/axis.py

@@ -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)


The rcParam is already validated.

oscargus · 2023-06-22T13:28:13Z

lib/matplotlib/text.py

+        if m is None:
+            m = "default"
+        else:
+            _api.check_in_list(("anchor", "default"), rotation_mode=m)


If this change is OK one should probably clarify that setting None will return "default" in the doc-string.

jklymak · 2023-06-24T20:12:45Z

lib/matplotlib/axis.py

@@ -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()


I guess I thought clear did a lot 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.

The thing is that Axis.clear is called from Axes.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.

jklymak

Modulo fixing the doc string

oscargus · 2023-06-25T11:27:58Z

I have updated the doc-string.

I would just like to highlight another change here that may or may not be controversial. Passing None to set_rotation_mode will now make get_rotation_mode return "default" rather than None. This is somewhat consistent with many other function where passing None just really sets it to a default value. Notable, no other code changes was required so one can assume that most code checks for "anchor" and then handles the two other case as the "else". This simplifies the argument checking a bit for a very commonly used function (every Tick and label).

oscargus · 2023-06-25T12:17:06Z

lib/matplotlib/axis.py

+        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``.


Not possible to link as there are three Axes.clear methods and I wanted to clearly state the difference between Axes.clear and Axis.clear.

tacaswell · 2023-07-05T19:25:51Z

lib/matplotlib/spines.py

+        from library code where it is known that the Axis is cleared separately.
+        """
+        self._position = None  # clear position
+


Suggested change

self.stale = True

I'll try to do a follow up PR, I suspect most of the clear methods do not mark the artist as stale when they should....

lib/matplotlib/axis.py

jklymak · 2023-07-06T16:16:11Z

@oscargus please self merge if you are OK with @tacaswell's changes!

oscargus added the Performance label Jun 21, 2023

oscargus force-pushed the noclearonregister branch 4 times, most recently from a15b0ec to 8107d05 Compare June 21, 2023 18:02

oscargus force-pushed the noclearonregister branch from 8107d05 to 01822a2 Compare June 21, 2023 19:03

oscargus force-pushed the noclearonregister branch 4 times, most recently from f07c0b0 to 2aa5cdd Compare June 21, 2023 19:42

oscargus force-pushed the noclearonregister branch from 2aa5cdd to 1652026 Compare June 21, 2023 20:55

oscargus marked this pull request as ready for review June 21, 2023 21:43

jklymak reviewed Jun 22, 2023

View reviewed changes

oscargus marked this pull request as draft June 22, 2023 07:57

oscargus force-pushed the noclearonregister branch from 1652026 to 0c072a5 Compare June 22, 2023 11:49

oscargus marked this pull request as ready for review June 22, 2023 12:15

oscargus changed the title ~~Do not clear Axis when registering spine~~ Only clear Axis once when creating an Axes Jun 22, 2023

oscargus force-pushed the noclearonregister branch from 0c072a5 to 651ba19 Compare June 22, 2023 13:18

oscargus commented Jun 22, 2023

View reviewed changes

oscargus mentioned this pull request Jun 22, 2023

Do not check value of parameters from rcParam (and check rcParam befo… #26167

Open

5 tasks

oscargus added this to the v3.8.0 milestone Jun 22, 2023

oscargus force-pushed the noclearonregister branch from 651ba19 to 7c9b3ee Compare June 22, 2023 20:14

jklymak reviewed Jun 24, 2023

View reviewed changes

jklymak approved these changes Jun 24, 2023

View reviewed changes

oscargus force-pushed the noclearonregister branch from 7c9b3ee to 6ae3134 Compare June 25, 2023 11:23

Do not clear Axis when registering spine

8a0ad0d

oscargus force-pushed the noclearonregister branch from 6ae3134 to 8a0ad0d Compare June 25, 2023 12:15

oscargus commented Jun 25, 2023

View reviewed changes

oscargus mentioned this pull request Jul 4, 2023

Improve performance of plt.clf() #26243

Closed

1 task

tacaswell reviewed Jul 5, 2023

View reviewed changes

tacaswell reviewed Jul 6, 2023

View reviewed changes

lib/matplotlib/axis.py Show resolved Hide resolved

DOC: add version added note to new clear kwarg

d4f7531

tacaswell approved these changes Jul 6, 2023

View reviewed changes

tacaswell reviewed Jul 6, 2023

View reviewed changes

lib/matplotlib/axis.py Outdated Show resolved Hide resolved

STY: remove whitespace

b86c8fd

oscargus merged commit d2fc3a4 into matplotlib:main Jul 7, 2023

oscargus deleted the noclearonregister branch July 7, 2023 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only clear Axis once when creating an Axes #26164

Only clear Axis once when creating an Axes #26164

oscargus commented Jun 21, 2023 •

edited

Loading

jklymak commented Jun 21, 2023

oscargus commented Jun 21, 2023 •

edited

Loading

oscargus commented Jun 21, 2023

jklymak Jun 22, 2023

oscargus Jun 22, 2023

rcomer Jun 22, 2023

oscargus Jun 22, 2023

oscargus commented Jun 22, 2023

oscargus Jun 22, 2023

oscargus Jun 22, 2023

oscargus Jun 22, 2023

jklymak Jun 24, 2023

oscargus Jun 24, 2023

jklymak left a comment

oscargus commented Jun 25, 2023

oscargus Jun 25, 2023

tacaswell Jul 5, 2023

tacaswell Jul 6, 2023

jklymak commented Jul 6, 2023

Only clear Axis once when creating an Axes #26164

Only clear Axis once when creating an Axes #26164

Conversation

oscargus commented Jun 21, 2023 • edited Loading

PR summary

PR checklist

jklymak commented Jun 21, 2023

oscargus commented Jun 21, 2023 • edited Loading

oscargus commented Jun 21, 2023

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

oscargus commented Jun 22, 2023

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 left a comment

Choose a reason for hiding this comment

oscargus commented Jun 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Jul 6, 2023

oscargus commented Jun 21, 2023 •

edited

Loading

oscargus commented Jun 21, 2023 •

edited

Loading