Added function to calculate richardson number. #1346
Conversation
|
Hi @joernu76 thank you for the contribution! Most everything looks good and the failing tests are not related to your PR. Your PR calculates the gradient Richardson number and is not exactly the same as the Bulk Richardson number (which is what is addressed in #628 and #647). In order to keep these two different methods of calculating a Richardson value, how about naming the function in your PR Other thoughts @jthielen? |
There was a problem hiding this comment.
Looks good overall! I agree with @kgoebber's comment regarding this being gradient (or flux as Holton puts it) Richardson number.
Just a few suggestions to incorporate @kgoebber's renaming suggestion, and a few clean-ups to make the code style (e.g., argument names and order, documentation conventions) more consistent.
joernu76
force-pushed
the
richardson
branch
2 times, most recently
from
8dc0195 to
fffa7ff
Compare
|
I agree with all the comments. Thanks for the helpful suggestions. I added all the commits using the github buttons (nice, I am using mostly open source gitlab). I typically would rebase the commits to the HEAD manually, but I assume this can be done from the GUI by the merger, if requested. |
dopplershift
added
Area: Calc
|
(Well for some reason it didn't ask me to leave an overall review comment). Apologies on the delay in feedback. This looks really good. Just a couple minor issues to resolve. Feel free to rebase the commits together if you like, or we can just squash them together when we merge. |
|
One other thing, can you add the name of the function to |
|
Will provide something today! |
Apply suggestions from code review Fixed flake8 errors of pipeline and accomodated further suggestions. Rebased to fix merge conflicts Co-Authored-By: Jon Thielen <github@jont.cc> See Unidata#647 and Unidata#628
|
Some CI stages fail, but I doubt that this is caused by my changes. |
There was a problem hiding this comment.
The changes look good to me!
@dopplershift Are the Travis and codecov failures due to the recent Cartopy v0.18.0 release?
|
@jthielen sigh Looks like it. |
I'll merge over CI failures.
See #647 and #628
Description Of Changes
Checklist