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

Allow PW calculation when NaNs present #1188

Merged
merged 2 commits into from Oct 14, 2019
Merged

Allow PW calculation when NaNs present #1188

merged 2 commits into from Oct 14, 2019

Conversation

zbruick
Copy link
Contributor

@zbruick zbruick commented Oct 2, 2019

Description Of Changes

Remove NaNs if present in dewpoint or pressure and calculate PW.

Checklist

@zbruick zbruick added Area: Calc Pertains to calculations Type: Bug Something is not working like it should labels Oct 2, 2019
@zbruick zbruick added this to the 0.12 milestone Oct 2, 2019
dopplershift
dopplershift previously approved these changes Oct 2, 2019
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.

Looks good. Makes sense as trapz is not NaN-aware. Is the data in the test just copied from the values in #1184?

@zbruick
Copy link
Contributor Author

zbruick commented Oct 2, 2019

Yes the test case is the one provided in #1184

@zbruick
Copy link
Contributor Author

zbruick commented Oct 2, 2019

Also should we be doing this for CAPE since it uses the same logic? @akrherz you have a profile you mentioned for this?

@dopplershift
Copy link
Member

Maybe we should refactor the integration with NaN handling and unit support into a separate (private) helper? I'd like to avoid playing whack-a-moledata-handling-bug.

@zbruick
Copy link
Contributor Author

zbruick commented Oct 2, 2019

Was wondering the same thing...can play around and see if we can get something more universal.

@akrherz
Copy link
Contributor

akrherz commented Oct 2, 2019

Also should we be doing this for CAPE since it uses the same logic? @akrherz you have a profile you mentioned for this?

When I run my example in #1184 without filtering into CAPE, it does not traceback, but I get a ton of warnings

/opt/miniconda3/envs/prod/lib/python3.6/site-packages/metpy/calc/tools.py:181: RuntimeWarning: invalid value encountered in greater
  mask = sign_change > 0
/opt/miniconda3/envs/prod/lib/python3.6/site-packages/pint/quantity.py:1193: RuntimeWarning: invalid value encountered in less
  return op(self._magnitude, other._magnitude)
/opt/miniconda3/envs/prod/lib/python3.6/site-packages/metpy/calc/tools.py:183: RuntimeWarning: invalid value encountered in less
  mask = sign_change < 0
/opt/miniconda3/envs/prod/lib/python3.6/site-packages/metpy/calc/thermo.py:1505: RuntimeWarning: invalid value encountered in greater
  keep_idx = np.ediff1d(x.magnitude, to_end=[1]) > 1e-6
/opt/miniconda3/envs/prod/lib/python3.6/site-packages/metpy/calc/tools.py:800: RuntimeWarning: invalid value encountered in less
  return (a < value) | np.isclose(a, value, **kwargs)
/opt/miniconda3/envs/prod/lib/python3.6/site-packages/metpy/calc/tools.py:778: RuntimeWarning: invalid value encountered in greater
  return (a > value) | np.isclose(a, value, **kwargs)

@zbruick
Copy link
Contributor Author

zbruick commented Oct 4, 2019

Took a swing at this helper...tried to make it as general as possible while not being overly complicated. Added it to PW and all CAPEs for now.

@dopplershift dopplershift modified the milestones: 0.12, 0.11.1 Oct 9, 2019
dopplershift
dopplershift reviewed Oct 10, 2019
View reviewed changes
src/metpy/calc/tools.py Outdated
Takes a variable number of arguments
Returns a dictionary with arrays in the same order as provided
"""
no_nan_variable = nan_variable[~np.isnan(nan_variable) & ~np.isnan(pressure)]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if the docstring is out of date or not, but I do think it would be good to work with multiple variables at once. In fact, that might actually be necessary if you get data where dewpoint and temperature have nans in different spots. What about:

def remove_nans(*variables):
    # Determine mask joint across all variables
    
    mask = None
    for v in variables:
        if mask is None:
            mask = np.isnan(v)
        else:
            mask |= np.isnan(v)

    # Mask everyone with that joint mask
    ret = []
    for v in variables:
        ret.append(v[~mask])
    return ret

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this...this was what I was intending but didn't know how to do. Haven't used |= before - very handy here!

dopplershift
dopplershift reviewed Oct 11, 2019
View reviewed changes
src/metpy/calc/thermo.py Outdated
@@ -407,6 +407,8 @@ def lfc(pressure, temperature, dewpt, parcel_temperature_profile=None, dewpt_sta
parcel_profile

"""
_, temperature = _remove_nans(pressure, temperature)
pressure, dewpt = _remove_nans(pressure, dewpt)
Copy link
Member

Choose a reason for hiding this comment

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

Should you be able to do this now:

pressure, temperature, dewpt = _remove_nans(pressure, temperature, dewpt)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll apply that to this function and the rest of the thermo calc that I applied this to.

@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2019

This pull request introduces 2 alerts when merging f8e06b4 into a928abf - view on LGTM.com

new alerts:

  • 2 for Module-level cyclic import

dopplershift
dopplershift approved these changes Oct 14, 2019
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.

Not sure what LGTM is really complaining about. This looks good.

@dopplershift dopplershift merged commit ce82267 into Unidata:master Oct 14, 2019
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

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

Successfully merging this pull request may close these issues.

precipitable_water fails with NaN dew point values in the profile
3 participants
@zbruick @dopplershift @akrherz