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.

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

BUG: make annotate_sphere and annotate_arrow safe when run after plot invalidation #4699

Merged
merged 4 commits into from
Oct 11, 2023
Merged

BUG: make annotate_sphere and annotate_arrow safe when run after plot invalidation #4699

merged 4 commits into from
Oct 11, 2023

Conversation

chrishavlin
Copy link
Contributor

Closes #4698

@chrishavlin
Copy link
Contributor Author

chrishavlin commented Oct 10, 2023 •
edited

didn't have a chance to run tests locally (other than verifying the reproducer script now works)... but probably should pass? don't think we have a lot of test coverage for this callback beyond simple unit tests.

Also -- should I add a new test (probably cant get to that til tomorrow)?

@chrishavlin
Copy link
Contributor Author

oh and note that I played around with storing the scaled_radius as a new attribute that it could use on the second pass through but then it got a bit too complicated thinking through cases where it might need to be re-calculated. So went with the simple route of always calculating it.

@neutrinoceros
Copy link
Member

Thank you for taking this on ! I think a new test would be useful too.

@chrishavlin chrishavlin changed the title BUG: avoid overwriting radius in annotate_sphere (SphereCallback) BUG: make annotate_sphere and annotate_arrow safe when run after plot invalidation Oct 11, 2023
@chrishavlin
Copy link
Contributor Author

added a test and fixed the ArrowCallback as well

@neutrinoceros neutrinoceros added this to the 4.3.1 milestone Oct 11, 2023
neutrinoceros
neutrinoceros reviewed Oct 11, 2023
View reviewed changes
yt/visualization/plot_modifications.py Outdated Show resolved Hide resolved
yt/visualization/tests/test_callbacks.py Outdated
p = SlicePlot(ds, "z", ("gas", "density"))
p.annotate_sphere([0.5, 0.5, 0.5], 0.1)
p.set_font_size(24)
assert_fname(p.save(prefix)[0])
Copy link
Member

Choose a reason for hiding this comment

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

I think we could speed up these tests by calling p.render instead of p.save here (yes, that's also true for a lot of existing tests in this module, but of course it's way out of scope to deal with everything here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good!

chrishavlin and others added 2 commits October 11, 2023 10:31
@chrishavlin @neutrinoceros
Apply suggestions from code review
380a98c 
Co-authored-by: Clément Robert <cr52@protonmail.com>
@chrishavlin
render rather than save in test
ba94b11
@neutrinoceros neutrinoceros enabled auto-merge (squash) October 11, 2023 15:51
@neutrinoceros neutrinoceros merged commit 21da682 into yt-project:main Oct 11, 2023
13 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/yt that referenced this pull request Oct 11, 2023
@chrishavlin @meeseeksmachine
Backport PR yt-project#4699: BUG: make annotate_sphere and annotate_a…
cacbe9c 
…rrow safe when run after plot invalidation
@meeseeksmachine meeseeksmachine mentioned this pull request Oct 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neutrinoceros neutrinoceros neutrinoceros approved these changes

Assignees
No one assigned
Labels
bug viz: 2D
Projects
None yet
Milestone
4.3.1
Development

Successfully merging this pull request may close these issues.

BUG: annotating a sphere then setting font size in a plot results in a crash
2 participants
@chrishavlin @neutrinoceros