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

Fix up internal multiplication by units #1819

Merged
merged 5 commits into from Apr 27, 2021

Conversation

dopplershift
Copy link
Member

Description Of Changes

This cleans up multiplication by units internally within MetPy by eliminating all places where we multiply arrays by units on the right. This is motivated by a user-reported issue with get_layer and masked arrays (Unidata e-support GSX-461780).

In order to catch all of these and ensure that we don't add more, I have created a custom flake8 plugin for MetPy and added it to CI, as suggested in #1818. Right now this just checks for multiplication by units on the right, but it should make it easier to add additional similar checks in the future. The plugin allows for the "normal" multiplication when using constants (or nan like np.nan).

Checklist

@dopplershift dopplershift added this to the 1.0.1 milestone Apr 21, 2021
@dopplershift dopplershift added Area: Calc Pertains to calculations Type: Bug Something is not working like it should labels Apr 21, 2021
@dopplershift
Copy link
Member Author

After some performance testing by @dcamron :

Performance Benchmarks

we're going to ban all uses of multiplication by units in the library code. Not only are they fraught with compatibility problems, they're universally slower. The flake8 plugin will catch all instances and suggest using Quantity instead. The code in tests/ will be allowed to stay as is because the performance isn't as important, and quite frankly I don't feel like changing ~730 of these occurrences manually.

@jthielen
Copy link
Collaborator

...must resist urge to figure out where the bottleneck is...

In any case, this sounds like a good change to me. Only concern is that a * units('whatever') -> units.Quantity(a, 'whatever') should not be promoted as a universal suggestion/rule, as doing this with upcast types (e.g., xarray DataArrays) or Quantities themselves (xref hgrecco/pint#1231) will break things terribly. Using the constructor directly definitely seems like the best option for construction when a is a downcast type.

@dopplershift
Copy link
Member Author

@jthielen If I had to guess, there's a bunch of dispatch taking place in *, figuring out the proper final type and who's handling the multiplication, whereas Quantity() just directly stores whatever you pass as the underlying data. Good point about the other types, that reinforces our thoughts that we shouldn't update examples & tutorials.

@dopplershift dopplershift force-pushed the units-multiply branch 2 times, most recently from af22e7f to d334d6c Compare April 24, 2021 00:50
.github/workflows/linting.yml Outdated Show resolved Hide resolved
tools/flake8-metpy/flake8_metpy.py Outdated Show resolved Hide resolved
This gives us a custom flake8 plugin we can use to detect custom errors,
style violations that we want. Right now this checks for multiplication
by units on the right.
These are problematic with masked arrays. Also, using the Quantity()
constructor is faster across the board. Therefore, just get rid of all
the places where we multiply by units.

This also adds a test for get_layer where it was failing with masked
arrays, which is what originally motivated this.
Use as a local plugin rather than needing to install and have the
entrypoint detected. Also, shorten code to MPY to be compliant with
future requirement for 3 characters. These are based on feedback from
flake8 dev.
@dopplershift dopplershift marked this pull request as ready for review April 26, 2021 18:31
@dcamron dcamron merged commit 350a989 into Unidata:main Apr 27, 2021
@dopplershift dopplershift deleted the units-multiply branch April 27, 2021 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Calc Pertains to calculations Type: Bug Something is not working like it should
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom linting tooling
4 participants