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.

Sign up for GitHub

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

Jump to bottom

Fixed a problem with diff'ing masked_arrays with units. #1345

Merged
merged 1 commit into from
Mar 31, 2020
Merged

Fixed a problem with diff'ing masked_arrays with units. #1345

merged 1 commit into from
Mar 31, 2020

Conversation

joernu76
Copy link
Contributor

See issue #983

Description Of Changes

Checklist

  • Closes #xxxx
  • Tests added
  • Fully documented

@CLAassistant
Copy link

CLAassistant commented Mar 30, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@dopplershift
Copy link
Member

@joernu76 Thanks for the contribution! Do you happen to have an example of what failed before, and works after this change? Would you be willing to add that as a test?

@dopplershift dopplershift added Area: Calc Pertains to calculations Area: Units Pertains to unit information Type: Bug Something is not working like it should labels Mar 30, 2020
@dopplershift dopplershift added this to the 1.0 milestone Mar 30, 2020
@joernu76
Copy link
Contributor Author

I had a failure when computing the first derivative of a masked_array with units using the metpy.calc.tools first_derivative function. I would assume that this would affect also any of your functions that make use of first_derivative().
I can see if I can provide that as a test case, if you require that. Will look into your test infrastructure and update the merge request. Is it sufficient to provide a merge request with fix and test, or do you require first a commit with a failing test and the fix in a separate one?

@joernu76
Fixed a problem with diff'ing masked_arrays with units.
503d45f 
The issue occurs only, when "x" in first_derivative is a masked array
with units. As all arrays stemming from NetCDF access are basically
masked arrays, we do not have non-masked arrays in our code, even though
the mask is almost always 'False'.

I added a test case demonstrating the problem, it fails without the
contained fix.

See issue Unidata#983
@joernu76 joernu76 requested a review from dcamron as a code owner March 31, 2020 08:43
dopplershift
dopplershift approved these changes Mar 31, 2020
View reviewed changes
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks for the contribution, and thanks for adding the test!

@dopplershift dopplershift merged commit 3aa0118 into Unidata:master Mar 31, 2020
@dopplershift dopplershift modified the milestones: 1.0, 0.12.1 Apr 17, 2020
@dopplershift dopplershift mentioned this pull request Apr 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dopplershift dopplershift dopplershift approved these changes

@dcamron dcamron Awaiting requested review from dcamron

Assignees
No one assigned
Labels
Area: Calc Pertains to calculations Area: Units Pertains to unit information Type: Bug Something is not working like it should
Projects
None yet
Milestone
0.12.1
Development

Successfully merging this pull request may close these issues.
3 participants
@joernu76 @CLAassistant @dopplershift