-
Notifications
You must be signed in to change notification settings - Fork 413
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
Conversation
53c69e8
to
b1858e0
Compare
After some performance testing by @dcamron : 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 |
...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 |
@jthielen If I had to guess, there's a bunch of dispatch taking place in |
af22e7f
to
d334d6c
Compare
b316473
to
023cce8
Compare
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.
023cce8
to
6be2517
Compare
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