Refactor steps blending for process-pool / MPI usage#552
Draft
ladc wants to merge 17 commits into
Draft
Conversation
…member. This will relax the constraint that n_ens_members >= n_model_members (NWP members). Useful when parallelising blending via a process pool or MPI.
…g at process level).
…_index_offset for improved parallel processing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #552 +/- ##
==========================================
+ Coverage 84.17% 84.18% +0.01%
==========================================
Files 170 170
Lines 14985 14996 +11
==========================================
+ Hits 12614 12625 +11
Misses 2371 2371
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* Add optional argument complevel to initialize_forecast_exporter_netcdf. Setting this to a lower value significantly speeds up netcdf writing * Add tests with different complevel (0-9).
Disregard stored skill if they don't match. They will get overwritten eventually.
…offset to determine this.
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors pysteps.blending.steps so the blended STEPS forecast can be driven one NWP member at a time from a process pool / MPI worker. A model_index_offset is threaded through forecast() and StepsBlendingConfig so each worker can index into the correct climatological skill entries, and a write_skill flag lets only one worker update the shared skill file. The forecast() docstring is shortened to delegate most parameter descriptions to StepsBlendingConfig, and clim.py is hardened against past-skill files whose shape no longer matches the requested configuration. Unrelated to that purpose, initialize_forecast_exporter_netcdf gains a configurable complevel.
Changes:
- Add
model_index_offset/write_skilltoStepsBlendingConfigandforecast(), use the offset when looking up NWP skill, and gatesave_skillonwrite_skill. - Improve the error message when too many NWP models are supplied, deduplicate the
forecast()docstring, and validate past-skill shape inpysteps.blending.clim. - Expose a
complevelargument oninitialize_forecast_exporter_netcdfand extendtest_exporters.py/test_blending_steps.pyparametrizations accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pysteps/blending/steps.py | Adds offset/write-skill plumbing, updates error messages, rewrites forecast() docstring to point to StepsBlendingConfig. |
| pysteps/blending/clim.py | Discards past-skill arrays whose ndim/n_models/n_cascade_levels no longer match the request. |
| pysteps/io/exporters.py | Replaces hard-coded zlib complevel=9 with a configurable parameter. |
| pysteps/tests/test_blending_steps.py | Extends parametrization with model_index_offset and adds a regression test for the "too many NWP models" error. |
| pysteps/tests/test_exporters.py | Adds complevel to the netCDF exporter test matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2143
to
+2146
| n_model=( | ||
| worker_state.mapping_list_NWP_member_to_ensemble_member[j] | ||
| + self.__config.model_index_offset | ||
| ), |
Comment on lines
+3495
to
+3502
| All other arguments (``n_cascade_levels``, ``blend_nwp_members``, | ||
| ``single_member_mode``, ``noise_method``, ``noise_stddev_adj``, | ||
| ``ar_order``, ``weights_method``, ``timestep_start_full_nwp_weight``, | ||
| ``conditional``, ``probmatching_method``, ``mask_method``, | ||
| ``resample_distribution``, ``smooth_radar_mask_range``, ``seed``, | ||
| ``num_workers``, ``fft_method``, ``domain``, ``outdir_path_skill``, | ||
| ``filter_kwargs``, ``noise_kwargs``, ``mask_kwargs``, ``callback``, | ||
| ``return_output``, ``measure_time``) are passed through unchanged. |
Comment on lines
+3466
to
+3470
| Time step of the motion vectors (minutes). | ||
| issuetime: datetime | ||
| is issued. | ||
| Issue time of the forecast. | ||
| n_ens_members: int | ||
| The number of ensemble members to generate. This number should always be | ||
| equal to or larger than the number of NWP ensemble members / number of | ||
| NWP models. | ||
| Passed to :class:`StepsBlendingConfig` as ``n_ens_members``. |
| fill_value=None, | ||
| scale_factor=None, | ||
| offset=None, | ||
| complevel=9, |
Contributor
Author
There was a problem hiding this comment.
This is from merging master into this branch. Why is this shown as a difference wrt master?
Comment on lines
+2143
to
+2146
| n_model=( | ||
| worker_state.mapping_list_NWP_member_to_ensemble_member[j] | ||
| + self.__config.model_index_offset | ||
| ), |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…e use separate directories per NWP member for the skill + adapt tests.
Comment on lines
+568
to
+597
| """Simulate one process-pool worker holding a single NWP model slice. | ||
|
|
||
| This mirrors the one-per-proc mode in multiprocessing.py: each worker | ||
| receives one model's precip/velocity slice, its own outdir_path_skill | ||
| sub-directory, and write_skill=True only for the designated skill writer | ||
| (one per NWP model). With 2 NWP models and 2 STEPS members, both | ||
| workers are skill writers (one each). The forecast output shape must | ||
| be correct regardless of the write_skill flag. | ||
| """ | ||
| test_steps_blending( | ||
| n_models=1, | ||
| timesteps=3, | ||
| n_ens_members=1, | ||
| n_cascade_levels=6, | ||
| nowcasting_method="steps", | ||
| mask_method=None, | ||
| probmatching_method=None, | ||
| blend_nwp_members=False, | ||
| weights_method="spn", | ||
| decomposed_nwp=True, | ||
| expected_n_ens_members=1, | ||
| zero_radar=False, | ||
| zero_nwp=False, | ||
| smooth_radar_mask_range=0, | ||
| resample_distribution=False, | ||
| vel_pert_method=None, | ||
| max_mask_rim=None, | ||
| timestep_start_full_nwp_weight=None, | ||
| write_skill=write_skill, | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The idea is that the STEPS blended forecast should be called with only 1 NWP member at a time when using process-level parallelisation. The user must slice the NWP models before passing them to the forecast method.
To ensure that the correct climatological weights, etc., are stored/retrieved, each process (NWP member) should write to its own directory. Not that the user should slice the NWP array per member and pass it to each process if they want to do a single member per process.
Using a separate
write_skillflag prevents multiple processes from writing to the same skill file.Also removed some duplicate documentation from the STEPS blending file that remained after @sidekock's refactoring work.
Some remaining things to check