Skip to content
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 postprocessing plugins #405

Merged
merged 40 commits into from
Mar 5, 2025
Merged

Conversation

joeycasey87
Copy link
Contributor

Added an infrastructure.py file to the postprocessing folder which should operate in the same way as the interface file does for the importers in the io folder. The postprocessor file is currently effectively empty as no postprocessor plugins have been added yet. A line has been added to the init file so that it should act similar to the init file in the io folder.

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

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

Project coverage is 84.55%. Comparing base (9dc68c5) to head (f871b4f).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
pysteps/postprocessing/interface.py 88.57% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #405      +/-   ##
==========================================
+ Coverage   84.23%   84.55%   +0.31%     
==========================================
  Files         160      162       +2     
  Lines       13242    13458     +216     
==========================================
+ Hits        11155    11379     +224     
+ Misses       2087     2079       -8     
Flag Coverage Δ
unit_tests 84.55% <93.38%> (+0.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dnerini dnerini changed the title Added structure for the creation of postprocessing plugins Add diagnostic plugins Aug 5, 2024
@joeycasey87
Copy link
Contributor Author

Hi @dnerini, could you please review this pull request when you have the time? The extended version of the cookiecutter plugin template which can be used to create importer, diagnostic postprocessing, and nowcast plugins is nearly finished but I require the postprocessing interface to be merged to ensure that everything is behaving correctly when tested. Cheers

@ladc
Copy link
Contributor

ladc commented Aug 20, 2024

Thanks for this contribution! We discussed how to make it more generic and @joeycasey87 will try to refactor it tomorrow to allow for various kinds of post-processors including new ensemblestats and bias correction methods, for example. To be continued.

@ladc ladc changed the title Add diagnostic plugins Adapt pysteps to allow for postprocessing plugins (diagnostics and ensemblestats for now) Aug 27, 2024
@ladc ladc changed the title Adapt pysteps to allow for postprocessing plugins (diagnostics and ensemblestats for now) Adapt pysteps to allow for postprocessing plugins Aug 27, 2024
@dnerini
Copy link
Member

dnerini commented Sep 8, 2024

what's the status of this PR? should we mark it as a draft?

@ladc ladc requested a review from FelixE91 January 14, 2025 14:07
joeycasey87 and others added 14 commits January 14, 2025 15:14
Also reformatted with black. Tests were added for the get_method and diagnostics_info functions in the postprocessing interface. Tests for the discover_diagnostics function will be written once these changes have been merged as then the cookiecutter plugins can then be properly tested.
Changed the test file to match the test updated pysteps test file
These were necessary for the IO plugins, but less so for the diagnostics.
Avoid duplicate code, refactor into functions.
Also fix a small typo causing a bug: postprocessor_s_
@ladc ladc force-pushed the postprocessor_plugin branch from d9b593d to 3586f56 Compare January 14, 2025 14:17
@ladc
Copy link
Contributor

ladc commented Jan 14, 2025

Apologies for the force-push, but I have done a rebase to the latest master version. The plan is now to clean up some of the dummy code (maybe move this into test, but most likely remove it entirely), add the necessary test(s) in test_plugins_support.py and hopefully merge it into the master branch soonish, thanks in advance to @FelixE91 #nopressure

For the tests, we're now only checking the importer cookiecutter template. The new precipitation type plugin was created using the diagnostics template (I suppose), so we should add a test for this new template ( see also https://github.com/pySTEPS/pysteps/blob/e3325854dae006b1e78eb43a6d2203f2b0b71560/pysteps/tests/test_plugins_support.py#L17C1-L17C20 )

Question: do we really need a separate template for all the plugin types? (Diagnostics, ensemblestats ... which are all in fact postprocessors)? Could we have one for any kind of postprocessors? Or even better, one cookiecutter template for all kinds of plugins which could even expose multiple functions (e.g. diagnostics and visualisation)? Not urgent at all.

@ladc ladc force-pushed the postprocessor_plugin branch from 51f9642 to 1305e88 Compare February 25, 2025 13:41
@ladc ladc self-requested a review February 26, 2025 14:38
Copy link
Contributor

@ladc ladc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost done, some final cleanup needed. There's also still some duplicate code but that will take a bit more time to simplify.

@ladc
Copy link
Contributor

ladc commented Feb 26, 2025

Also, by updating the cookiecutter template I just realised that we broke the standard pysteps tests, so best to merge these changes in quite fast.

@FelixE91
Copy link
Contributor

Additional tests are successful. CodeCov's getting closer to the target value. Still some work to do before we can merge into main.

Copy link
Contributor

@FelixE91 FelixE91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's decide together whether we want to merge it like this or make some of the recommended changes first. @dnerini @ladc

Copy link
Contributor

@ladc ladc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, this is ready to merge

@ladc ladc merged commit 5fd4dc8 into master Mar 5, 2025
9 of 10 checks passed
@ladc ladc deleted the postprocessor_plugin branch March 5, 2025 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants