search

pySTEPS / pysteps

Python framework for short-term ensemble prediction systems.
https://pysteps.github.io/
BSD 3-Clause "New" or "Revised" License
471 stars 168 forks source link

Adapt pysteps to allow for nowcast plugins. #418

Open Loickemajou opened 3 months ago

Loickemajou commented 3 months ago

Creating a DGMR plugin to be intergrated into the pysteps nowcasts methods.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 81.39535% with 8 lines in your changes missing coverage. Please review.

Project coverage is 83.99%. Comparing base (917c83b) to head (45070d4). Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
pysteps/nowcasts/interface.py 78.37% 8 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #418 +/- ## ========================================== + Coverage 83.88% 83.99% +0.10% ========================================== Files 160 160 Lines 12780 12924 +144 ========================================== + Hits 10720 10855 +135 - Misses 2060 2069 +9 ``` | [Flag](https://app.codecov.io/gh/pySTEPS/pysteps/pull/418/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pySTEPS) | Coverage Δ | | |---|---|---| | [unit_tests](https://app.codecov.io/gh/pySTEPS/pysteps/pull/418/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pySTEPS) | `83.99% <81.39%> (+0.10%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pySTEPS#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Loickemajou commented 3 months ago

apologies for the slow action on this, I left few comments to clarify certain parts, but otherwise this is looking good!

So the actual plugin would be https://github.com/pySTEPS/pysteps-dgmr-nowcasts right? is everything ready on that side?

Hello @dnerini , thanks for reviewing the code.

Concerning the plugin, everything is ok, just left to include a test for the plugin.

ladc commented 3 months ago

I think this can soon be merged, although it shows that the current way the plugins are structured is hitting its limits. It leads to a lot of duplicate code and maintainability issues, and also it's not so flexible. I'll open a separate issue about that.

These modifications allow the pysteps-nowcast-dgmr plugin and other future nowcast plugins to be detected if installed.

dnerini commented 2 months ago

I think this can soon be merged, although it shows that the current way the plugins are structured is hitting its limits. It leads to a lot of duplicate code and maintainability issues, and also it's not so flexible. I'll open a separate issue about that.

Hi @ladc, could you please elaborate on the issues that you mentioned? anything we can do to improve it or should we rather go ahead with this PR as it is now and improve it in a separate effort?

ladc commented 2 months ago

I think the current state is fine, it enables nowcast plugins in pysteps, opening the door for all kinds of data driven methods. The reason why I said it's hitting its limits is because another internship student, @viktor40, wanted to create a plugin which included both post-processing and visualisation functionality, but the way the plugin system is currently designed only allows a single functionality type to be added. I suppose it's not too difficult to extend it but currently the internships are mostly finished so we'll have to do that at a later time. I would also need to add a test for the nowcast loader, but for that we need to update the cookiecutter - so I propose to go ahead and merge this in, if it's ok for everyone.