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
Absolute momentum units bug fix #1956
Conversation
Ensures that units are provided to `u` and `v` when calculating normal_component.
src/metpy/calc/cross_sections.py
Outdated
u = u.metpy.convert_units('m/s') | ||
v = v.metpy.convert_units('m/s') |
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.
Maybe change these to e.g. u.metpy.quantity()
? Then the last line of the function could be:
return (norm_wind + f * distance).metpy.convert_units('m/s')
src/metpy/calc/cross_sections.py
Outdated
x = x.metpy.convert_units('meters') | ||
y = y.metpy.convert_units('meters') |
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.
Could maybe just entirely eliminate these lines? Not quite sure.
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.
"It looks like removing them doesn't break any tests"... famous last words.
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.
😆 It's an "opportunity" to test better if that's the case. 😉
New test makes sure that absolute_momentum() works when using units from the xarray DataArray `units` attribute.
Implements @jthielen's solution to issue Error in absolute_momentum (bug in normal_component?) #1948, which converts
u
andv
to m/s before passing tonormal_component()
.Refactors one line of
absolute_momentum()
, but I'm not familiar with the absolute momentum calculation. I'm willing to include more refactoring in this PR, but I would need some more specific direction on what to adjust/remove.Checklist