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

Incorrect instruction for assigning units in check_unit error message #2334

Closed
jthielen opened this issue Feb 4, 2022 · 1 comment · Fixed by #2415
Closed

Incorrect instruction for assigning units in check_unit error message #2334

jthielen opened this issue Feb 4, 2022 · 1 comment · Fixed by #2415
Labels
Area: Units Pertains to unit information Type: Bug Something is not working like it should
Milestone

Comments

@jthielen
Copy link
Collaborator

jthielen commented Feb 4, 2022

[...] #2333 made me notice that the following error message in check_units is incorrect:

MetPy/src/metpy/units.py

Lines 262 to 264 in 94ef0e0

msg += ('\nAny variable `x` can be assigned a unit as follows:\n'
' from metpy.units import units\n'
' x = units.Quantity(x, "m/s")')

This approach to assigning units will not (and is never intended to) work for xarray data types. [...]

Originally posted by @jthielen in #2185 (comment)


Based on #2185 (comment), I was thinking the error message should probably be updated to something more like

For a given variable `x`, if `x` is an xarray DataArray, you can assign units as follows:
    from metpy.units import units
    x = x * units("m/s")
If `x` is a NumPy Masked Array, you can assign units instead as:
    x = units.Quantity(x, "m/s")
If `x` is a scalar or any other common array type, either of the above approaches will
work. For more information, see the "Working with Units" tutorial in MetPy's documentation.

The order of the two suggestions can definitely be flipped based on what we expect/want a user to default to.

@jthielen jthielen added Type: Bug Something is not working like it should Area: Units Pertains to unit information labels Feb 4, 2022
@jthielen jthielen added this to the 1.3.0 milestone Feb 4, 2022
@jthielen
Copy link
Collaborator Author

Conclusion from telecon discussion: check against type of x for it being a masked array to do the constructor suggestion, otherwise suggest multiplication approach. In both cases, include some hint towards the docs.

dopplershift added a commit to dopplershift/MetPy that referenced this issue Apr 6, 2022
This make the message appropriate to whether we were given a MaskedArray
or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Units Pertains to unit information Type: Bug Something is not working like it should
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant