Closed timbonin closed 2 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 97.17%. Comparing base (
84a8944
) to head (7951819
).:exclamation: Current head 7951819 differs from pull request most recent head 7451ec9. Consider uploading reports for the commit 7451ec9 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
This would be a great addition to IMPROVER, and very interesting to see what diagnostic schemes are in use elsewhere.
At the Australian Bureau of Meteorology we have an alternative algorithm that makes use of the instantaneous cloud physics thunder parameter (Bright et al. 2005) along with accumulated precipitation, although we are in the process of developing improved formulations using additional convective parameters and we may also implement this eventually in the IMPROVER framework. As such, I would request we may be more careful in naming the Python modules or functions/classes here to make them a little less generic, e.g. class LightningMultivariateProbability_USAF2024
or similar
Thanks @ivorblockley. We'll try to have a conversation within the Met Office regarding the naming convention issue that you raise here, just to try to come up with a recommendation, as this could potentially impact other algorithms already within the IMPROVER codebase.
I am open to changing the name of the module to whatever we decide is best going forward. To be honest, our working name for this internally was to call it LightningProbability_USAF (without the year), but after reviewing the other names we decided to change the name as no other ones were named specific to any one agency. Whatever we decide as a group is best, we are happy to go with.
Thank you @gavinevans for the thorough review and helping me walk through the process of my first IMPROVER PR. I think I've learned from this experience and hopefully subsequent ones will be smoother, as I'll be aware of the common issues raised during this one (e..g, comprehensive unit tests, checksums, documentation, etc).
I've 'resolved' all the conversations mostly so that I could track which ones I've addressed. Feel free to review them and make sure I've adequately addressed all the comments/concerns and let me know if you have any other suggestions.
Ok, I think we've addressed all the comments and concerns! I've updated the unit tests for metadata, created acceptance tests that are not idealized that, ensured the validity time was at the end of the period bounds, and made the additional minor changes.
Further, we did make a few minor changes to the algorithm (e.g., we changed the sign on CIN), as conventionally CIN is given as <=0 whereas the regression equations used assumed it was >=0. We realized we were doing a pre-processing step on our CIN values to reverse the convention, but we've moved this reversal directly into the algorithm so that any users can pass in CIN cubes as they are standardly defined.
Thanks for all the updates @timbonin 👍 . I've added one query to the associated test data PR: https://github.com/metoppv/improver_test_data/pull/44, but otherwise this PR looks fine to me now.
Thanks for all the updates @timbonin 👍 . I've added one query to the associated test data PR: metoppv/improver_test_data#44, but otherwise this PR looks fine to me now.
I've just addressed the other query on the acceptance test data, so I think we should be good to go. I'll double check with you one more time.
Thanks @timbonin for addressing all my feedback.
Thank you @ivorblockley and @bayliffe for the additional comments! I appreciate the feedback, and think I've addressed all the concerns, mainly with regard to unit throughout.
I've rebased to master, as some of the unit tests weren't passing on Github actions. This was related to some changes made to the set_up_variable_cube
test helper earlier this week. Thus, I made minor changes to the unit testing framework to use the helper appropriately and everything seems to be passing now.
@bayliffe, I'll let you take one more look though to make sure you have no other concerns. Again, thank you for the feedback!
Adding the lightning algorithm that MIT LL has added developed based on current USAF documentation of their operational algorithm used in house. There will be a companion PR going up shortly for acceptance tests accompany this.
I spent a fair amount of time making sure the code style and all the other checks passed, so that should make this a bit easier to review. Open to any suggestions on if things should be reformatted. We added the logic of the code to the existing lightning.py file as we thought it made sense, but have a separate CLI call for invoking the algorithm 'lightning_multivariate_probability'.
Let me know if you have any comments/concerns.
Acceptance test data PR: https://github.com/metoppv/improver_test_data/pull/44
Testing:
CLA