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

Refactor overlay drawing to use fewer artists #1614

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Conversation

psavery
Copy link
Collaborator

@psavery psavery commented Nov 10, 2023

For all overlay types, we were using more artists than we needed to. For powder overlays, for instance, we would have one artist per line. For Laue and rotation series overlays, we would have one artist per spot and one artist per range! This could easily result in hundreds of artists.

However, if everything about the artists (including the style) are identical, except for the data, we are able to merge them together into a single artist. This can make matplotlib run much faster.

For line artists, we can have a single artist draw every powder overlay line - we just insert a [nan, nan] row in between lines in the data. Different artists are used for different styles (i. e., merged ranges are red instead of green, so they get a different artist, and highlighted data/ranges are a different color too, so they get a different artist). But we end up using only about 5 line artists in total for the main canvas, per overlay and per detector (each detector is currently rendered separately).

For scatter (path) artists, we ought to just pass every single spot to a single call to scatter().

These changes speed up rendering of the overlays significantly, especially when there were many lines or spots being drawn. This is a great step toward better interactivity.

@psavery psavery requested a review from bnmajor November 10, 2023 17:18
@bnmajor
Copy link
Collaborator

bnmajor commented Nov 10, 2023

I am testing with the tardis_simulated_powder_and_laue state file from the examples repo and I am seeing weirdness with the masking:

powder_masking

Seems like everything between the lines is being masked now. Additionally the Azimuthal Average plot is also being masked it seems - I don't think that is the expected behavior though, am I correct?

@pep8speaks
Copy link

pep8speaks commented Nov 10, 2023

Hello @psavery! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-11-10 20:05:07 UTC

For all overlay types, we were using more artists than we needed to.
For powder overlays, for instance, we would have one artist per line.
For Laue and rotation series overlays, we would have one artist per
spot and one artist per range! This could easily result in hundreds
of artists.

However, if everything about the artists (including the style) are
identical, except for the data, we are able to merge them together
into a single artist. This can make matplotlib run much faster.

For line artists, we can have a single artist draw every powder overlay
line - we just insert a `[nan, nan]` row in between lines in the data.
Different artists are used for different styles (i. e., merged ranges
are red instead of green, so they get a different artist, and highlighted
data/ranges are a different color too, so they get a different artist).
But we end up using only about 5 line artists in total for the main canvas,
per overlay and per detector (each detector is currently rendered separately).

For scatter (path) artists, we ought to just pass every single spot to
a single call to `scatter()`.

These changes speed up rendering of the overlays significantly, especially
when there were many lines or spots being drawn. This is a great step
toward better interactivity.

Signed-off-by: Patrick Avery <[email protected]>
Since the `rbnds` are no longer sorted in start/stop pairs, we have
to add some logic to handle it better.

We don't want to go back to the start/stop pair setup because it
was error-prone and would make overlay rendering a little slower.

Signed-off-by: Patrick Avery <[email protected]>
@psavery psavery force-pushed the fewer-overlay-artists branch from 8c2b880 to cf9b726 Compare November 10, 2023 19:52
Signed-off-by: Patrick Avery <[email protected]>
@psavery
Copy link
Collaborator Author

psavery commented Nov 10, 2023

@bnmajor Can you give it a try again? It should be fixed now.

Copy link
Collaborator

@bnmajor bnmajor left a comment

Choose a reason for hiding this comment

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

Awesome!

This doesn't matter for most examples, which have all zeros for
their tvec_s. However, it is important to include this for the
examples that do.

Signed-off-by: Patrick Avery <[email protected]>
@psavery psavery merged commit 0f3ff71 into master Nov 10, 2023
9 checks passed
@psavery psavery deleted the fewer-overlay-artists branch November 10, 2023 20:10
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.

3 participants