Skip to content

MNT: Fix double evaluation of _LazyTickList #28911

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 1, 2024
Merged

MNT: Fix double evaluation of _LazyTickList #28911

merged 1 commit into from
Oct 1, 2024
+10 −4

Conversation

timhoffm
Copy link
Member

Closes #28908.

It seems the read-access to instance.majorTicks per instance .majorTicks.append() still triggers the descriptor, even though instance.majorTicks = [] previously should have rebound the name. For details see #28908.

@timhoffm timhoffm marked this pull request as draft September 30, 2024 20:25
@anntzer
Copy link
Contributor

anntzer commented Sep 30, 2024

I figured out the reason for the double __get__: the tick = instance._get_tick(major=...) call actually undoes the initialization done the line just above, because it calls (after a few layers) Spine.get_spine_transform which ultimately calls reset_ticks, which deletes the majorTicks attribute...
So the fix here, which re-defines again the attribute after it has been deleted, seems OK to me (it could use a comment, though...).

@timhoffm
Copy link
Member Author

timhoffm commented Sep 30, 2024 •
edited
Loading

Thanks for figuring out. I suspect it's a design flaw that we may call reset_ticks from within _get_tick(). But that's for another time.

This PR is a good step forward. It isolates the _LazyTickList against funny behavior from _get_tick(). And more importantly, it significantly speeds up Axes creation. As mentioned in #28908:

before (ms) after (ms) change
plt.subplots() 38±2 31±1 -18%
plt.subplots(10, 10) 1420±13 1063±7 -25%

I believe this is worth mentioning in "What's new". If possible, I'd like to get this into 3.10.

@timhoffm timhoffm marked this pull request as ready for review September 30, 2024 23:24
@timhoffm timhoffm added this to the v3.10.0 milestone Sep 30, 2024
@timhoffm
MNT: Fix double evaluation of _LazyTickList
f1b0a28 
Closes matplotlib#28908.

The final instance.majorTicks list must be assigned after running
`_get_tick()` because that may call `reset_ticks()`, which invalidates
a previous set list. We still temporarily assign an empty tick list
to instance.majorTicks, because `_get_tick()` may rely on that
attibute to exist.
@anntzer anntzer merged commit 14dda09 into matplotlib:main Oct 1, 2024
42 of 43 checks passed
@timhoffm timhoffm deleted the mnt-lazyticklist branch October 1, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer approved these changes

@ksunden ksunden ksunden approved these changes

Assignees
No one assigned
Labels
topic: ticks axis labels
Projects
None yet
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.

[Bug]: Possible performance issue with _LazyTickList
3 participants
@timhoffm @anntzer @ksunden