-
Notifications
You must be signed in to change notification settings - Fork 169
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
Adapt pysteps to allow for nowcast plugins. #418
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this block? the dgmr plugin, if installed, will be discovered below in discover_nowcasts()
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see, is it because the function is called
forecast
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you removed that part, @Loickemajou , does it mean that you found an alternative solution?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.