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

Fixing minor LFC bug #491

Merged
merged 2 commits into from Jul 22, 2017
Merged

Fixing minor LFC bug #491

merged 2 commits into from Jul 22, 2017

Conversation

mwilson14
Copy link
Contributor

Adding a fix so that, in the case where the parcel path never intersects the profile, the surface data point is ignored in determining whether the parcel path is ever warmer than the environment (and thus, whether the LFC exists). Currently, in some cases the parcel is identified as warmer at the surface than the environment, resulting in a spurious LFC being identified at the same pressure as the LCL.

@dopplershift dopplershift changed the title Fixing minor LFC typo Fixing minor LFC bug Jul 12, 2017
@dopplershift dopplershift added Area: Calc Pertains to calculations Type: Bug Something is not working like it should labels Jul 12, 2017
@dopplershift dopplershift added this to the Summer 2017 milestone Jul 12, 2017
@dopplershift
Copy link
Member

Can you show an example of this? I'm not clear on why we should be ignoring the surface point. Seems like in the case of surface saturation and a completely unstable profile, the surface point should count. Is this another case where truncation (or whatnot) is causing problems?

Also, if this goes in, I'd like a comment above the line indicating why [1:] is necessary so that we don't revisit this in the future. You should also add a test validating this behavior.

@mwilson14
Copy link
Contributor Author

Sure! Today's sounding from Denver was where I first noticed it. It might be a truncation problem somewhere, because the surface point should always be equal to the first point of the parcel profile. For the case of surface saturation and a completely unstable profile, the parcel path would still be warmer than the environment on the levels immediately above the surface, which would cause the LFC to be set equal to the LCL, which for a case of surface saturation would be the surface. I'll look and see if it's a truncation issue, since if it is addressing that would be the ideal way to go.
lfc_bug

@dopplershift
Copy link
Member

So is the problem that the LFC should be at the surface?

@mwilson14
Copy link
Contributor Author

Nope, in this case there should be no LFC. I did figure out that it is a precision issue, however. The surface temperature in this particular case is 20.6 C going in to parcel profile, and the surface point of the parcel path is 20.600000000000023 C.

@mwilson14
Copy link
Contributor Author

It appears that the spurious digits get added on somewhere in parcel profile, or in converting parcel profile's output to Celsius.

@jrleeman
Copy link
Contributor

I have to think about this a bit it terms of both why no LFC is the correct result and the conversion issues, but sounds related (maybe) to what @tjwixtrom found.

baby_what

@mwilson14
Copy link
Contributor Author

That's what we're thinking back here in Boulder. Just to clarify things, the sounding from Denver that I posted is the one that should have no LFC, and Ryan is correct in saying that for his hypothetical example where the sounding is absolutely unstable and saturated from the surface up the LFC should be at the surface. Also, that's a hilarious gif.

@jrleeman
Copy link
Contributor

So I think that @dopplershift and I can agree that dropping the surface point comparison isn't the desired behavior as the LFC could be at the surface. So we're back to a precision issue then.

@mwilson14
Copy link
Contributor Author

Yeah, that makes sense. I think part of this goes into the whole idea of where we want to define the LFC (at the base of the largest area of CAPE vs the first time the parcel path is warmer than the environment, etc.) What if we defined the LFC as the point where the parcel path crosses over the temperature trace in a positive direction nearest to the largest positive difference between the parcel temperature and the environment? Theoretically, that should do a decent job of placing the LFC at the base of the largest area of CAPE, and I think I might know how to code it up.

@jrleeman
Copy link
Contributor

But is it really at the base of the largest area or is it the first crossing? I've got a way to potentially handle precision, but if it's not needed that's fine.

Copy link
Contributor

@jrleeman jrleeman left a comment

Choose a reason for hiding this comment

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

I think the answer isn't to skip the first point, but maybe do a comparison with np.isclose(). Does that make sense here?

@dopplershift
Copy link
Member

Wouldn't the new _greater_or_close() you just added be the right answer?

@jrleeman
Copy link
Contributor

Absolutely - some asynchronous comm here as that was added post that comment, but yes, it is the exact right tool for the job @mwilson14

@mwilson14
Copy link
Contributor Author

Sounds good!

@mwilson14
Copy link
Contributor Author

Less_or_close has been implemented in LFC.

@@ -232,7 +232,7 @@ def lfc(pressure, temperature, dewpt):
direction='increasing')
# Two possible cases here: LFC = LCL, or LFC doesn't exist
if len(x) == 0:
if np.any(ideal_profile > temperature):
if np.any(_less_or_close(ideal_profile[1:], temperature[1:]) == False):
Copy link
Member

Choose a reason for hiding this comment

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

I think the idea was to use _less_or_close instead of slicing off the first point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes because the first point could be the answer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. That was a typo.

Copy link
Member

Choose a reason for hiding this comment

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

Also should be:

if not np.any(...)

rather than doing ==False

@mwilson14
Copy link
Contributor Author

Don't merge this yet, I may have just found a bug.

@mwilson14
Copy link
Contributor Author

Now it should be ok.

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.

Minor nit, otherwise we're good to go here.

if np.any(ideal_profile > temperature):
if np.all(_less_or_close(ideal_profile, temperature)):
return np.nan * pressure.units, np.nan * temperature.units
# LFC doesn't exist
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this on the same line as the else? It makes it clearer what block we're referring to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it!

@dopplershift
Copy link
Member

Look good @jrleeman ?

@dopplershift
Copy link
Member

I think rebasing on master might help Travis succeed.

@dopplershift
Copy link
Member

Test failure due to broken link in docs, which is fixed on master. Merging...

@dopplershift dopplershift merged commit 56f2681 into Unidata:master Jul 22, 2017
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.

None yet

3 participants