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

allow annotate_grids to force cmap to respect max_levels #4658

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

zingale
Copy link
Member

@zingale zingale commented Sep 1, 2023

PR Summary

At the moment, when you use annotate_grids() to show boxes and compare simulations with different
numbers of refinement levels, the colors don't correspond to one another across simulations. For example,
here are 4 different simulations. One has base + 1 level, then next 2 levels, then 3, and finally 4 levels of refinement
(all are jumps of 2x). Notice that the color for level 1 is not the same across all simulations:

subch_Temp_res_compare_orig

Here's the output with this PR if I pass use_max_level_in_cmap=True to annocate_grids().

subch_Temp_res_compare

Note: I am unsure why the original behavior was desired, but I left it in place.

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@zingale zingale added enhancement Making something better viz: 2D labels Sep 1, 2023
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Although the change is simple, I'm not sure I understand the approach: it would seem reasonable to me to consider the current behaviour is buggy; within this framing, I think we don't need to keep "backward compatibility", so the introduction of a new argument seems superfluous.

If anyone can convince me that I'm being mislead and that we do need to preserve existing behaviour, then I'd strongly recommend that any new argument (especially booleans) be made keyword only and documented.

@zingale
Copy link
Member Author

zingale commented Sep 4, 2023

I also don't understand the current way it works, but I didn't want to remove it in case someone was relying on "spacebar heating"

@neutrinoceros
Copy link
Member

Nice reference !
I think we could use a third pair of eyes here. As far as I am concerned, I feel like this kind of "breaking" change could even be acceptable if shipped in a feature release. Let's aim this at the upcoming 4.3 release and see what others think !

@neutrinoceros neutrinoceros added this to the 4.3.0 milestone Sep 4, 2023
@matthewturk
Copy link
Member

Yeah, we can remove the old behavior, I think. Thanks for submitting this!

@zingale
Copy link
Member Author

zingale commented Sep 6, 2023

okay, I'll update the PR to just change the current behavior

@neutrinoceros neutrinoceros added bug and removed enhancement Making something better labels Sep 6, 2023
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Thanks @matthewturk and @zingale !

@neutrinoceros neutrinoceros enabled auto-merge (squash) September 6, 2023 14:11
@neutrinoceros neutrinoceros merged commit 27751ba into yt-project:main Sep 6, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants