Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gridliner: don't destroy and recreate artists #2252

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Sep 28, 2023

Rationale

@greglucas has asked for this at least twice!

When re-drawing the GridLiner, update its existing LineCollection and Text objects rather than creating new ones. This was trivial for the LineCollections but a bit more complicated for the labels since we might have different numbers of them e.g. after zooming. Clearly Matplotlib's Axis has already solved that problem so I got my inspiration from there. We now keep two lists of labels: _all_labels is the list of labels that have been drawn at some point. _labels is the list that were used at the most recent draw (and still gets cleared out and repopulated each time).

Goes on top of #2249. Only the third commit is new.

Implications

@rcomer
Copy link
Member Author

rcomer commented Sep 30, 2023

Putting this one in draft as it will need rebasing once #2249 is merged.

@rcomer rcomer marked this pull request as draft September 30, 2023 19:50
@rcomer rcomer marked this pull request as ready for review October 2, 2023 16:28
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this out locally zooming and panning around and it seems quite responsive to me, nice work!

One minor pre-commit failure to fix before merging.

@rcomer
Copy link
Member Author

rcomer commented Oct 3, 2023

Apologies, I don't know how I managed to push that failure as I have pre-commit installed locally. I should have checked the CI myself though.

@rcomer rcomer marked this pull request as draft October 3, 2023 19:00
@rcomer
Copy link
Member Author

rcomer commented Oct 3, 2023

I broke the minimum versions tests. In fact I broke them at #2249: Figure.draw_without_rendering and the layout keyword for figure, etc. were introduced at mpl v3.5. Should I fix them all here or make a separate PR?

@rcomer
Copy link
Member Author

rcomer commented Oct 3, 2023

I note that mpl v3.5 is two years old next month, so the easiest fix would be to move the pin.

The problem calls are only in the tests.

@greglucas
Copy link
Contributor

I broke the minimum versions tests. In fact I broke them at #2249: Figure.draw_without_rendering and the layout keyword for figure, etc. were introduced at mpl v3.5. Should I fix them all here or make a separate PR?

:( I should have checked that during the review but I was only looking at the ones without min-version specified. That is the problem when you have external issues causing red x's throughout the test suite.

I'd say it should probably be a separate commit, but I'm fine having it in this PR if that is easier than a new PR.

I note that mpl v3.5 is two years old next month, so the easiest fix would be to move the pin.
The problem calls are only in the tests.

I'd say that bumping the pin is fine then. We are not going to release within the next month. Note that you should also probably remove any other MPL3_4 checks there are (if any) and remove that code too, and if so then a new PR might be worthwhile.

@dopplershift
Copy link
Contributor

👍 to what @greglucas said above.

@rcomer
Copy link
Member Author

rcomer commented Oct 4, 2023

I decided to put it in a separate PR #2258

@rcomer rcomer closed this Oct 5, 2023
@rcomer rcomer reopened this Oct 5, 2023
@rcomer rcomer marked this pull request as ready for review October 5, 2023 16:42
@rcomer
Copy link
Member Author

rcomer commented Oct 5, 2023

I think this one is now ready to go. The only change since @greglucas' approval is the flake8 fix and I have also checked that all the test runs have exactly ten failures.

@greglucas
Copy link
Contributor

Still looks good to me. I verified the test runs as well.

@greglucas greglucas merged commit 93dd3b0 into SciTools:main Oct 6, 2023
2 of 22 checks passed
@rcomer rcomer deleted the gridliner-no-destroy branch October 6, 2023 08:49
@QuLogic QuLogic added this to the Next Release milestone Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants