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

BUG: Fix passing masked arrays to CAPE/CIN calcs #1516

Merged
merged 1 commit into from Sep 30, 2020

Conversation

dopplershift
Copy link
Member

Description Of Changes

This is to explicitly fix passing masked arrays to surface_based_cape_cin, but really fixes some low-level helper functions, so this likely fixes all of the CAPE/CIN calculations.

Checklist

@dopplershift dopplershift added this to the 1.0 milestone Sep 29, 2020
@dopplershift dopplershift added Area: Calc Pertains to calculations Type: Bug Something is not working like it should labels Sep 29, 2020
dcamron
dcamron previously approved these changes Sep 30, 2020
Copy link
Member

@dcamron dcamron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to me like this is clean and ready to go!

This is to explicitly fix passing masked arrays to
surface_based_cape_cin, but really fixes some low-level helper
functions, so this likely fixes all of the CAPE/CIN calculations.
@ghost
Copy link

ghost commented Sep 30, 2020

Congratulations 🎉. DeepCode analyzed your code in 3.446 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@dopplershift
Copy link
Member Author

Rebased to fix conflict. DeepCode is wrong.

@dopplershift dopplershift merged commit bf77d24 into Unidata:master Sep 30, 2020
@dopplershift dopplershift deleted the fix-1496 branch September 30, 2020 05:33
@akrherz
Copy link
Contributor

akrherz commented Sep 30, 2020

Peanut gallery here and a day late, two dollars short with my comment, but I am curious this change. The MetPy code base seems to be rather consistent with the style of

blah = values * units.something

This change seems to switch the order to fix this bug, but how does it fix the bug? Should all of metpy switch to that ordering or is it not supposed to matter, which in that case, this fix should not matter? This seems arbitrary :)

@jthielen
Copy link
Collaborator

jthielen commented Sep 30, 2020

@akrherz Long story short is that right now this is an upstream issue with regards to masked arrays, numpy/numpy#15200, which shows up a lot and so we have to work around it often.

blah = values * units.something

is indeed Pint's canonical way of doing constructing a Quantity, and how it should work, but it breaks if the type of values doesn't properly defer to upcast types like Pint Quantities/Units and instead tries handling the operation itself. I prefer

blah = units.Quantity(values, 'something')

instead of

blah = units.something * values

when values is of a non-cooperative type, but either work, since now Pint is handling the operation.

@akrherz
Copy link
Contributor

akrherz commented Sep 30, 2020

Hi @jthielen, thanks. I think that's my point then.

If this is brittle,

blah = values * units.something

and either of the following two are not brittle

blah = units.Quantity(values, 'something')
blah = units.something * values

Then all of Metpy and slack-jawed yocals like me should start using one of the two non-brittle methods?

@dopplershift
Copy link
Member Author

Yes...and no. Yes, because everyone's code will be more likely to work when using netCDF4-python. No, because we really want to the original to work since it maps most easily to our mental models and how we write values with units--and "just" need to fix upstream.

So as usual I end up battling between practicality and my platonic ideal that is just beyond my fingertips.

Or use xarray and it will "just work".

@dopplershift
Copy link
Member Author

dopplershift commented Sep 30, 2020

I should also add that this has become a much bigger problem because netCDF4-python went from "return masked arrays when necessary" to "return masked arrays always", and a lot of cases that worked fine before now suddenly are now needlessly broken by them.

Edit: not to say that returning masked arrays always was a bad choice (polymorphic returns having nothing to do with what you pass in is confusing to the users), just we now have people dealing with masked arrays when they don't need them. I'm of the opinion that masked_arrays from netCDF should always have been opt-in behavior.

@jthielen
Copy link
Collaborator

I should also add that this has become a much bigger problem because netCDF4-python went from "return masked arrays when necessary" to "return masked arrays always", and a lot of cases that worked fine before now suddenly are now needlessly broken by them.

Edit: not to say that returning masked arrays always was a bad choice (polymorphic returns having nothing to do with what you pass in is confusing to the users), just we now have people dealing with masked arrays when they don't need them. I'm of the opinion that masked_arrays from netCDF should always have been opt-in behavior.

And given how poorly maintained MaskedArrays seem to be within NumPy (with some suggestions even to move it to a third-party package), it's even more of a headache.

@akrherz
Copy link
Contributor

akrherz commented Sep 30, 2020

Thanks @dopplershift for indulging me here. I have seen a couple of other projects now emitting warnings for RHS multiplication support for their objects for similar reasons to this I think and was curious.

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.

surface_based_cape_cin fails with masked arrays
4 participants