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
[bugfix] Use axes unit in scale bar if set by user. #2989
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few suggestions. I'm happy to approve this once they are addressed and/or answered :)
if plot._axes_unit_names is not None and self.coeff is not None: | ||
self.unit = plot._axes_unit_names[0] | ||
# Nothing provided; identify a best fit distance unit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing you don't expect plot._axes_unit_names
and self.coeff
could be set to None independently, and maybe that's perfectly justified with the current state of the code base, but it looks like an implementation detail. I would feel more confident about this if/else
block if all possibilities were taken into account. I assume that in the conditions I'm describing this should throw an error.
min_scale = plot.ds.get_smallest_appropriate_unit( | ||
min_scale, return_quantity=True | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm puzzled by this bit... It doesn't look like it's used anywhere. I recon it was there before your patch, but while you're at it, I assume it's safe to clean this up :)
elif self.coeff is None: | ||
self.coeff = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So again, this was here before your patch, but I feel like it would be slightly more clear to have this as a default value in init(), because I'm worried that the block you're patching is getting a bit too large and harder to maintain. What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think since this is in response to some issues raised on the mailing list, we should err on the side of getting it in in its current state. @neutrinoceros would you be OK deferring the other fixes to later?
Honestly I don't trust us to remember this later, but if I misjudged the urgency of the fix, I'm fine with my nitpick remarks left unanswered :) |
@neutrinoceros I've added a todo item to address the issues you raised, which I'm hoping to get to after finishing grades today. |
PR Summary
This is in response to an old email thread. If the user makes a plot, sets the axes unit, and then adds a scale annotation with a coefficient, the most appropriate behavior is to use the axes unit set by the user. The current behavior is to instead figure out an appropriate unit on its own, but this calculation does not take into account the user-provided coefficient. In other words, the following script creates a scale bar with units of Mpc, when Mpccm/h is probably more appropriate.
This PR changes the behavior to use the axes unit in the case described above. This is a very minor issue, but I think this is the correct behavior.
PR Checklist
black --check yt/
isort . --check --diff
flake8 yt/
flynt yt/ --fail-on-change --dry-run -e yt/extern