-
Notifications
You must be signed in to change notification settings - Fork 415
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
Implement convective condensation level (CCL) #2550
Conversation
10e9c02
to
268874d
Compare
Fix docstring for ccl function Fix inline comment for ccl
@dopplershift @dcamron Hi MetPy developers -- I would really appreciate it if I could get some feedback on this PR. The math here is pretty straightforward:
The tests provide decent coverage for the function, although there are a few caveats. I basically grabbed some sounding data from Please let me know if there is anything else I can do to improve my code. Thanks for reviewing this PR! |
Thanks for the PR and the explanation! Grabbing arrays from siphon is perfect for the tests. I'll comment on explicitly testing height once I review, but you're right that it likely doesn't need it. I do plan on reviewing this and we can include it in for our feature release scheduled for the end of this month. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the terribly delayed response and review.
The code looks to be in great shape. I only have a question about a reference for the calculation so that I can validate the implementation against that. Otherwise, just a few minor doc formatting items.
Co-authored-by: Ryan May <rmay31@gmail.com>
Co-authored-by: Ryan May <rmay31@gmail.com>
Co-authored-by: Ryan May <rmay31@gmail.com>
df1e055
to
7a58f20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed derivation, which I went through and checked as well.
Then I realized we were doing this in SkewT
already, so you can make use of those functions instead. Do that and consider updating the docs like suggested, and this can go on in!
Oh, there's one more consideration: I'm not sold on the need to build in the mixed layer approach. We don't do it in much else (like Is the need for this that |
Co-authored-by: Ryan May <rmay31@gmail.com>
Co-authored-by: Ryan May <rmay31@gmail.com>
Thanks for the suggestion! I should have speculated that this was implemented somewhere in MetPy, but I would have never figured out what those I agree that we should simplify the function by not using the mixed layer approach, but I got this definition from AMS. There may be folks in AMS who want to adhere to their own definition of CCL, and I think the function should still be reasonably simple to use if we add an example to show what the default behavior looks like (i.e. use surface dewpoint instead of a mixed layer.) Again, I am good with going either direction, so it might be your call to decide which way is better for our users. And as a last note, I will be adding some examples after we have settled down on whether we should use a mixed layer approach. These examples will make life much easier for our users. |
And one last thing: I just realized that the unit handling in this function is terribly wrong but somehow escapes the test cases. It seems that |
Quick ping @dopplershift |
@Z-Richard based on our discussion on today's MetPy dev call (which anyone is welcome to join), we can keep the direct implementation of the mixed layer support since it seems like it would be a really common use case for the tool. So feel free to add some examples demonstrating this. I'll dig into the unit issue you encountered and see what's up there. |
@dopplershift Never mind the unit issue. It turns out that I should also add |
Ah, yeah the converter is just smart enough to first make sure to convert things to a common unit and then check the values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code here looks great. If you could add the examples you mentioned in the gallery (and maybe one in the docstring) this will be great to include in the next release.
I think it would be worth the effort to bundle |
One last question: should the docstring description
also reflect the fact that users can use a mixed layer dewpoint to compute |
The manual (4.18 and 4.19) says
I think it's fine to mention the option to use a mixed layer and still be consistent with the reference. |
This should be good to go in. Could you do a final check? |
Co-authored-by: Ryan May <rmay31@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, one last change to correct convective temperature.
Co-authored-by: Ryan May <rmay31@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks for working with us on this great contribution!
Description Of Changes
This PR implemented the convective condensation level (CCL), with options to use either the surface mixing ratio or the average mixing ratio over a shallow layer near the surface, according to the definition of CCL by AMS.
I also wrote a bunch of tests to make sure that what I implemented is correct using UWYO sounding. I can't find any existing software to compare my CCL calculations to, so I just eyeballed the desired values from the Skew-T diagrams.
Sorry for creating a new PR -- it turns out that I messed up with git for the last PR, and opening a new one seems the best option.
Checklist