Adapt pysteps to allow for nowcast plugins. #418
Conversation
…ds should be discovered by pysteps
…r and view the available nowcasts methods
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
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?
pysteps/nowcasts/__init__.py
Outdated
| try: | ||
| from dgmr_module_plugin import dgmr | ||
| except ImportError: | ||
| dgmr = None |
There was a problem hiding this comment.
why this block? the dgmr plugin, if installed, will be discovered below in discover_nowcasts()
There was a problem hiding this comment.
oh I see, is it because the function is called forecast and it should be added to a module named dgmr? https://github.com/pySTEPS/pysteps-dgmr-nowcasts/blob/c8903bfa689951c555cfb275fb2100e36a683e40/setup.py#L86
There was a problem hiding this comment.
oh I see, is it because the function is called
forecastand it should be added to a module nameddgmr? https://github.com/pySTEPS/pysteps-dgmr-nowcasts/blob/c8903bfa689951c555cfb275fb2100e36a683e40/setup.py#L86
Also, in the get_method, actually when the plugin is installed, the dgmr is discovered by pysteps and it is possible to
dgmr=get_method('dgmr').
Though is not quite a nice way, included this block so that when the plugin is installed we could called it directly by doing
from pysteps.nowcasts import dgmr.
Had no idea on how to do that differently and will be very delighted to have some suggestions.
There was a problem hiding this comment.
I see you removed that part, @Loickemajou , does it mean that you found an alternative solution?
There was a problem hiding this comment.
I see you removed that part, @Loickemajou , does it mean that you found an alternative solution?
Hello @dnerini
I have removed the conflicting dependencies and model weights to ensure that they do not interfere with PySTEPS during execution.
However, I have not yet found an alternative solution and would appreciate any suggestions or insights you might have to help improve the integration process.
Hello @dnerini , thanks for reviewing the code. Concerning the plugin, everything is ok, just left to include a test for the plugin. |
|
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. |
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? |
|
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. |
| # nowcasts methods available in the `nowcasts` package | ||
| available_nowcasts = [ | ||
| attr.split(".")[0] | ||
| for attr in os.listdir(" ".join(nowcasts.__path__)) |
There was a problem hiding this comment.
@Loickemajou Is there a more elegant way to detect all the nowcast methods that doesn't use os.listdir?
There was a problem hiding this comment.
Thanks @ladc, for the remark.
I will take time to look into alternative approaches and see if I can find a more elegant way to achieve this.
Creating a DGMR plugin to be intergrated into the pysteps nowcasts methods.