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

EL calculation #338

Merged
merged 3 commits into from Mar 8, 2017
Merged

EL calculation #338

merged 3 commits into from Mar 8, 2017

Conversation

jrleeman
Copy link
Contributor

@jrleeman jrleeman commented Mar 6, 2017

Pulls out the EL calculation from #322

@@ -190,3 +190,12 @@ def test_equivalent_potential_temperature():
t = 288. * units.kelvin
ept = equivalent_potential_temperature(p, t)
assert_almost_equal(ept, 315.9548 * units.kelvin, 3)

Copy link
Member

Choose a reason for hiding this comment

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

Missed a blank line (for pep8)


def test_el():
"""Test equilibrium layer calculation."""
levels = np.array([959., 779.2, 751.3, 724.3, 700., 269.]) * units.mbar
Copy link
Member

Choose a reason for hiding this comment

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

These are indented at 5 spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What the!?

@jrleeman
Copy link
Contributor Author

jrleeman commented Mar 6, 2017

Should we have a test for the no EL case as well? Seems like a good idea.

@dopplershift
Copy link
Member

I think that's an important edge case to verify it works--have you even tried that?

The existing 3 May 1999 data should suffice for this.

@jrleeman
Copy link
Contributor Author

jrleeman commented Mar 6, 2017

Yes, I did try it on real data with no problems. I'll add a test now.

@jrleeman jrleeman force-pushed the EL_Calculation branch 2 times, most recently from 73b17d4 to f2151c1 Compare March 6, 2017 21:35
@dopplershift
Copy link
Member

Weird about appveyor--I guess you need to resolve the merge conflict?

@jrleeman jrleeman force-pushed the EL_Calculation branch 3 times, most recently from 8bb3df6 to ee564f2 Compare March 6, 2017 22:27
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 nitpick, otherwise this looks good.

# If there is only one intersection, it's the LFC and we return None.
if len(x) <= 1:
return None, None

Copy link
Member

Choose a reason for hiding this comment

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

Can we nuke the extra blank line?

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.

Just found something to improve in the docs.

r"""Calculate the equilibrium level.

This works by finding the last intersection of the ideal parcel path and
the measured parcel temperature. If there is one or fewer intersections, there is
Copy link
Member

Choose a reason for hiding this comment

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

measured environmental temperature

@dopplershift dopplershift merged commit e31be5e into Unidata:master Mar 8, 2017
@jrleeman jrleeman deleted the EL_Calculation branch March 9, 2017 15:21
@dopplershift dopplershift modified the milestone: Spring 2017 Mar 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants