-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
WCSAxes: pre-compute the coord range to eliminate redundant transformations when drawing ticks/gridlines #16366
base: main
Are you sure you want to change the base?
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
👋 Thank you for your draft pull request! Do you know that you can use |
Nice! How much is the overall performance improvement with this for solar data? |
Is this performance covered by https://github.com/astropy/astropy-benchmarks/blob/main/benchmarks/visualization/wcsaxes.py ? If so, can slap a |
We might as well try that as we should at least see a small improvement even for non solar coordinates! |
FWIW the benchmark here doesn't indicate anything has changed in wcsaxes stuff but it can also be flaky. 🤷♀️ https://github.com/astropy/astropy/actions/runs/8911393283/job/24474385670?pr=16366 |
The improvements here are likely only for cases with slow coordinate transformations, which probably explains it. |
Okay, I had thought there'd be a gigantic performance gain for large images because I thought If we start with the same benchmark test but specify the overlay to be in
So, it looks like a ~10% increase in performance when there are only a few gridlines. This fractional benefit will be much smaller if there are lots of gridlines. |
Amusingly, it's possible to make the performance gains from this PR arbitrarily large by increasing |
Description
From #16362:
Inspired by @astrofrog, this PR pre-computes the coord range at just the right spot to exactly cover these four transformations, so now only one transformation is performed.
Fixes #16362