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

Main module blending #233

Merged
merged 96 commits into from
Jan 14, 2022
Merged

Conversation

RubenImhoff
Copy link
Contributor

@RubenImhoff RubenImhoff commented Aug 24, 2021

First 'steps' towards a main module for a blended nowcasts-NWP forecast following the original STEPS approach as described in Bowler et al. (2006) and Seed et al. (2013).

  • Working main module
  • Tests
  • Examples
  • Documentation
  • Probability matching following a different method
  • Implementation of blending weights as in Bowler et al. (2006)
  • Implementation of blending weights as in Seed et al. (2013)
  • Fixes to prability matching method, masking method and noisy output
  • blending.utils tests

Part of issue #212

cvelascof and others added 30 commits April 3, 2019 15:17
* Add xarray dependency

* MCH importer returns an xarray Dataset

* Remove plot lines

* Remove import

* Adapt readers to xarray format

* Rewrite as more general decorator

* Add missing metadata

* Adapt io tests

* Mrms bounding box (pySTEPS#222)

* Fix bounding box coordinates

* Add missing metadata

* Import xarray DataArray

Ignore quality field

* Black

* Do not hardcode metadata

* Address review comments by ruben

* Add a legacy option to the io functions

A "legacy" options is added to revert back the importers and readers behavior to version 1. This is a temporary solution to allow the examples, and other functions, to run as usual (v1.*).

Hopefully, this is will allow a easier transition into version 2 during the development process and will allow testing functions that were not updated to v2.

* Fix compatibility problems with tests

Many of the tests were updated to use the legacy data structures (v1). The tests that still contains issues, were tagged with a TODO message and they are skipped.

This will allow incremental changes to be tested in the new v2.

IMPORTANT: once the v2 branch is stable, we may remove the legacy compatibility from the code base and the tests.

* Update dependencies

* Ignore plugins test

Co-authored-by: Andres Perez Hortal <16256571+aperezhortal@users.noreply.github.com>
…on of skill values to climatological values for weights determination
@RubenImhoff
Copy link
Contributor Author

pystepsrc

You have included a new path_workdir. I'm not against, but what's its purpose? Perhaps you could include a short comment to help understand its scope?

Good point. We implemented this as a place to store the decomposed NWP fields and NWP velocity fields, but seeing how it used now, we can reach the same with 'just' the path_outputs in pystepsrc. @ladc, what do you think?

@RubenImhoff
Copy link
Contributor Author

In the blended forecast, I have the impression that something is a bit off with the NWP values, as I observed a strong increase of the intensities with leadtime. I anyway still need to have a closer look at your implementation of the masking and probability matching.

I completely forgot to have a new look at the examples forecast after the latest changes. The minor code changes will follow soon. Indeed, something seems to go wrong with either the masking or prob_matching.. I don't see the same effect (at least it is not as clear if it is there) in the test forecasts with the RMI data, so I wonder what goes wrong.

@RubenImhoff
Copy link
Contributor Author

RubenImhoff commented Dec 10, 2021

In the blended forecast, I have the impression that something is a bit off with the NWP values, as I observed a strong increase of the intensities with leadtime. I anyway still need to have a closer look at your implementation of the masking and probability matching.

I completely forgot to have a new look at the examples forecast after the latest changes. The minor code changes will follow soon. Indeed, something seems to go wrong with either the masking or prob_matching.. I don't see the same effect (at least it is not as clear if it is there) in the test forecasts with the RMI data, so I wonder what goes wrong.

While working on the tests for pysteps.blending.utils, I came across an error in pysteps.blending.utils. In the function stack_cascades, the mean and sigma could be set to 0.0 and 1.0 (I don't know why), which eventually gives wrong values after blending with NWP. As this only happened with the NWP component(s), the prob_matching was compensating for this and that is why we saw the effect with increasing time in the examples scripts. However, the changes above fix the problems (nevertheless, masking and prob_matching still require a critical look)! Also for the RMI test scripts the results look better:

BlendedForecast_202107041405

@dnerini
Copy link
Member

dnerini commented Dec 14, 2021

Very nice @RubenImhoff! The animation is very promising!

I'm having a closer look at the probability matching. Do I understand it right that you basically perform a linear blending of radar extrapolation and NWP based on the weights from the second cascade level in order to build the reference field for the probability matching? This is now a bit of a detail, but the problem with this is that you would loose the peak values, especially when both weights are around 0.5.

An alternative approach would be a simple resampling: for each grid point you sample either the radar or the NWP value (and this could be based on the same weights you are already using). This way you would build a new target distribution that is not smoother than the original radar and NWP fields.

I have some code for this (I used it in the EnKF approach). Should I give it a try to implement it in your code?

@RubenImhoff
Copy link
Contributor Author

An alternative approach would be a simple resampling: for each grid point you sample either the radar or the NWP value (and this could be based on the same weights you are already using). This way you would build a new target distribution that is not smoother than the original radar and NWP fields.

I have some code for this (I used it in the EnKF approach). Should I give it a try to implement it in your code?

Great idea, @dnerini, and thanks if you want to implement that!

@RubenImhoff
Copy link
Contributor Author

Yesterday, @ladc, @cvelascof and me had a nice talk with Alan Seed about the blending code and the weights implementation. He was happy to see that we're making this open source in pysteps and thought that the results so far look promising. Based on our talk, some main suggestions from Alan (and things to work on probably in the coming days):

  • Have a look at default skill values for cases where the weights implementation either becomes unstable or for when there is hardly any rain in the domain. This should ensure that the noise weights always increases with time and the nowcast weight decreases.
  • Have a look at the combined weight term of the nowcast and NWP component (prior to the noise weight estimation) and make sure this always decreases with lead time.
  • For the transformations, perhaps apply a log sine transformation which can include zeroes, thereby making the variance of the individual components more representative than with a threshold set (Alan said it is better not to mask the radar and NWP data with a given threshold prior to the blending). This is something we could implement. I wonder what the optical flow methods (and perhaps also the FFT) need, maybe we should at least keep a threshold for the motion estimations, but not for the remaining part of the blending?

@ladc
Copy link
Contributor

ladc commented Dec 17, 2021

pystepsrc

You have included a new path_workdir. I'm not against, but what's its purpose? Perhaps you could include a short comment to help understand its scope?

Good point. We implemented this as a place to store the decomposed NWP fields and NWP velocity fields, but seeing how it used now, we can reach the same with 'just' the path_outputs in pystepsrc. @ladc, what do you think?

Hi guys, sorry for the super late reply. I think we made the distinction because the one path is aimed at files for internal use, including input and output (also e.g. the weights file, pre-decomposed NWP, ...) and the other is where we store only the output products for users. But I also understand if you would prefer to reduce the number of different directories or parameters in the config file, and also store these in path_outputs.

@ladc
Copy link
Contributor

ladc commented Dec 17, 2021

That is not a bad idea, Daniele. How much time do you think we still need for v2? I'm thinking of our idea (at least @ladc and mine - but not limited to) to work towards a blending paper, including the open-source implementation of the STEPS code in pySTEPS and a hydrometeorological analysis. As long as we have a release ready by that time, it should suffice.

@ladc, what do you think?

The xarray overhaul (among other things) is indeed not a small task, and it takes some manpower to work on pysteps v2. If you think it's doable to integrate the blending into pysteps v1.6, then it might be better to go for that option indeed. I hope this does not create too much duplicate work?

Only changes I can think of, is that the importers and subsequent testing schemes are (partly) based on the xarray implementation.

@dnerini
Copy link
Member

dnerini commented Dec 19, 2021

The xarray overhaul (among other things) is indeed not a small task, and it takes some manpower to work on pysteps v2. If you think it's doable to integrate the blending into pysteps v1.6, then it might be better to go for that option indeed. I hope this does not create too much duplicate work?

It should be doable in my opinion. To simplify the task, I suggest to exclude the nwp importers from the branch and publish them as separate pysteps plugins: https://pysteps.readthedocs.io/en/latest/developer_guide/importer_plugins.html

The rest of the code is already very much in line with v1 I believe? I'd also suggest a minimal refactoring to reorganize the main method in few components (I'd be happy to give it a try).

If we all agree, I'd start refactoring this PR so that it can be merged into master rather than pysteps-v2.

@RubenImhoff very cool that you could exchange with Alan about your work! One comment from my side on your list of improvements: at this point I'd try to limit adding complexity to this implementation. We want a robust base blending method that works, possibly as close as possible to the reference publication. Further improvements can come incrementally later on (with your publication? ;-)), and as separate modules, to avoid adding too much complexity to the base method. For example: climatological skill. This is an important component for an operational system (it has to run in any situation), but I would not include it in the base method. Instead, we can make sure that the method can accept is as an optional argument, and leave to the user to compute it in his/her implementation. Same idea with the log sine transformation: would be nice to experiment with it, but this can be included in a separate PR.

@RubenImhoff
Copy link
Contributor Author

If we all agree, I'd start refactoring this PR so that it can be merged into master rather than pysteps-v2.

@RubenImhoff very cool that you could exchange with Alan about your work! One comment from my side on your list of improvements: at this point I'd try to limit adding complexity to this implementation. We want a robust base blending method that works, possibly as close as possible to the reference publication. Further improvements can come incrementally later on (with your publication? ;-)), and as separate modules, to avoid adding too much complexity to the base method. For example: climatological skill. This is an important component for an operational system (it has to run in any situation), but I would not include it in the base method. Instead, we can make sure that the method can accept is as an optional argument, and leave to the user to compute it in his/her implementation. Same idea with the log sine transformation: would be nice to experiment with it, but this can be included in a separate PR.

Sounds good, Daniele, and I agree! I'll upload a couple of minor changes and implement your idea to make the decomposed NWP input optional (so, you could also start with a non-decomposed NWP forecast) and then we're ready to go.

@RubenImhoff
Copy link
Contributor Author

I've just merged Daniele's great refactoring work. Should we continue the review here, or should I merge this branch in the steps_blending branch and renew that for pysteps 1.6 (and review from there)?

…ng changes to prevent errors in blending.utils
@dnerini
Copy link
Member

dnerini commented Jan 14, 2022

I've just merged Daniele's great refactoring work. Should we continue the review here, or should I merge this branch in the steps_blending branch and renew that for pysteps 1.6 (and review from there)?

Very good. Yes, please merge into steps_blending and we'll continue from there for the integration into master

@dnerini
Copy link
Member

dnerini commented Jan 14, 2022

I've just merged Daniele's great refactoring work. Should we continue the review here, or should I merge this branch in the steps_blending branch and renew that for pysteps 1.6 (and review from there)?

Very good. Yes, please merge into steps_blending and we'll continue from there for the integration into master

mmmh maybe that's not a good idea if we want to merge into master after that. Let's instead merge into a new branch from master, I'll do it now.

@dnerini dnerini changed the base branch from steps_blending to blending-v1 January 14, 2022 07:34
@dnerini dnerini merged commit d6652ea into pySTEPS:blending-v1 Jan 14, 2022
pysteps-v2 automation moved this from In progress to Done Jan 14, 2022
dnerini added a commit that referenced this pull request Jan 14, 2022
* First basic functions to implement STEPS blending

* Add compute of blend means,sigmas and recompose

* pysteps.io with xarray (#219)

* Add xarray dependency

* MCH importer returns an xarray Dataset

* Remove plot lines

* Remove import

* Adapt readers to xarray format

* Rewrite as more general decorator

* Add missing metadata

* Adapt io tests

* Mrms bounding box (#222)

* Fix bounding box coordinates

* Add missing metadata

* Import xarray DataArray

Ignore quality field

* Black

* Do not hardcode metadata

* Address review comments by ruben

* Add a legacy option to the io functions

A "legacy" options is added to revert back the importers and readers behavior to version 1. This is a temporary solution to allow the examples, and other functions, to run as usual (v1.*).

Hopefully, this is will allow a easier transition into version 2 during the development process and will allow testing functions that were not updated to v2.

* Fix compatibility problems with tests

Many of the tests were updated to use the legacy data structures (v1). The tests that still contains issues, were tagged with a TODO message and they are skipped.

This will allow incremental changes to be tested in the new v2.

IMPORTANT: once the v2 branch is stable, we may remove the legacy compatibility from the code base and the tests.

* Update dependencies

* Ignore plugins test

Co-authored-by: Andres Perez Hortal <16256571+aperezhortal@users.noreply.github.com>

* Add blend_optical_flow

* changes to steps blending procedure - weights according to adjusted BPS2006 method

* changes to blending procedures - adjust weights from original BPS2006 method

* Determine spatial correlation of NWP model forecast

* First attempt to make correlations and thus weights lead time dependent (in progress..)

* Change back to original BPS2006 blending formulation and add regression of skill values to climatological values for weights determination

* Reformat code with Black

* Skill score script imports climatological correlation-values from file now

* Small changes to skill score script

* Add skill score tests and an interface

* Add skill score tests and an interface

* Small change to docstring

* Bom import xarray (#228)

* Add import_bom_rf3  using xarray

* Add tests to xarray version

* Fix mrms importer tests

* Pass **kwargs to internal functions

* Add nwp_importers to read bom nwp sample data

* Add bom nwp data to source file

* Add tests for bom_nwp reader

* Fix pystepsrc

Co-authored-by: Andres Perez Hortal <16256571+aperezhortal@users.noreply.github.com>

* Functions to store and compute climatological weights (#231)

* Implement the functions get_default_weights, save_weights, calc_clim_weights.

These functions are used to evolve the weights in the scale- and skill-dependent blending with NWP in the STEPS blending algorithm. The current weights, based on the correlations per cascade level, are regressed towards these climatological weights in the course of the forecast.

These functions save the current and compute the climatological weights (a running mean of the weights of the past n days, where typically n=30). First daily averages are stored and these are then averaged over the running window of n days.

* Add tests for pysteps climatological weight io and calculations.

* Add path_workdir to outputs section in pystepsrc file and use it as a default path to store/retrieve blending weights.

* Minor changes to docstrings, changes to skill scores and testing scripts

* Completed documentation for blending clim module, cleanup.

Co-authored-by: RubenImhoff <r.o.imhoff@live.nl>

* Main blending module, first steps

* Add simple tests

* Minor changes to tester: velocity now based on rainfall field of NWP

* Add utilities to decompose, store and load NWP cascades for use in blending  (#232)

* First version of NWP decomposition

* Added saving to netCDF

* Completed functions for saving and loading decomposed NWP data

* Added example files for the decomposed NWP functions

* Added compatibility with numpy datetime

* Use default output path_workdir for tmp files in blending/utils.py.

* Update documentation of NWP decomposition functions in utils.py

Co-authored-by: Wout Dewettinck <wout.dewettinck@ugent.be>
Co-authored-by: wdewettin <87696913+wdewettin@users.noreply.github.com>

* Add importer for RMI NWP data (#234)

Add importer for netcdf NWP data from RMI using xarrays.

* Add test for RMI NWP data importer.

* Add entry for RMI NWP data in pystepsrc.

* Run black on everything: fix formatting.

* Add KNMI Harmonie NWP netcdf importer and tests (#235)

* Changes to v_models to make use of multiple timesteps. Changes in the velocity field over time in the NWP forecast will be taken into account now.

* Fixes for KNMI importer:

Add forgotten @postprocess_import()
Don't call dropna on NWP data.

* Avoid shadowing of pysteps.blending.utils by pysteps.utils

* First attempt for probability matching and masking utility; part 1

* Changes to prob matching and masking methods; part 2

* Prob matching and masking changes; part 3. Ready for testing with real data from here on

* Remove unnecessary print statements

* Cleanup imports

* More cleanup

* Update docstrings

* RMI importer for gallery example (will follow)

* Reprojection functionality (#236)

* Added Lesley's reprojection module to this branch

* Added compatibility for three-dimensional xarrays

* Add commentary to reprojection util

* Changes to make reprojection of KNMI data possible

* Changes after Daniele's review

* Add dependencies

* Changes to importers, see issue #215

* Add tests

* Fix some issues

* documentation

* Fixes for tests

* Set requirements again

* Some fixes

* Changes to nwp_importers after Carlos' response

* Remove wrong example script

* Remove rasterio dependencies from lists

* First try to prevent testing error

* Changes Daniele and fix knmi nwp importer

* Add rasterio to tox.ini

* Aesthetics

* rasterio import test

* Add rasterio to the test dependencies

* Reset try-except functionality for rasterio import

* Fix for failing test on windows python 3.6

* add importerskip rasterio

Co-authored-by: Wout Dewettinck <wout.dewettinck@ugent.be>

* Fixes in nwp importers

* Revert "Merge branch 'steps_blending' into pysteps-v2" (#239)

This reverts commit 2c639f8, reversing
changes made to bccb8fc.

* Merge latest version pysteps-v2 into steps_blending branch (#237)

* Update docstrings

* More cleanup

* Cleanup imports

* Cleanup imports

* More cleanup

* Update docstrings

* Update references

Mention the work of Ravuri et al (2021, Nature) as an example of work using cGANs to generate ensembles

* Clean up page

* Reprojection functionality (#236)

* Added Lesley's reprojection module to this branch

* Added compatibility for three-dimensional xarrays

* Add commentary to reprojection util

* Changes to make reprojection of KNMI data possible

* Changes after Daniele's review

* Add dependencies

* Changes to importers, see issue #215

* Add tests

* Fix some issues

* documentation

* Fixes for tests

* Set requirements again

* Some fixes

* Changes to nwp_importers after Carlos' response

* Remove wrong example script

* Remove rasterio dependencies from lists

* First try to prevent testing error

* Changes Daniele and fix knmi nwp importer

* Add rasterio to tox.ini

* Aesthetics

* rasterio import test

* Add rasterio to the test dependencies

* Reset try-except functionality for rasterio import

* Fix for failing test on windows python 3.6

* add importerskip rasterio

Co-authored-by: Wout Dewettinck <wout.dewettinck@ugent.be>

* Revert "Merge branch 'steps_blending' into pysteps-v2" (#239)

This reverts commit 2c639f8, reversing
changes made to bccb8fc.

Co-authored-by: ned <daniele.nerini@meteoswiss.ch>
Co-authored-by: dnerini <daniele.nerini@gmail.com>
Co-authored-by: Wout Dewettinck <wout.dewettinck@ugent.be>

* NWP skill calculation only within radar domain

* Update docs

* Add example for gallery examples

* Fix docstrings example

* Remove additional normalization step

* Fixes for the tests

* update docs

* changes to post-processing rainfall field and docstrings

* Update contributing guidelines (#241)

- Improve grammar.
- Make the guide more concise. Remove unused/unnecessary rules.
- Indicate more clearly which parts of the guidelines are inspired by other projects (before they were only mentioned at the end).
- Change "Travis-CI" references by "GitHub Actions".

* Advect noise cascade

* Allow for moving domain mask of extrapolation component

* minor fixes

* Linear blending (#229)

* Implemented linear blending function
* Added example file and test
* Added compatibility for NWP ensembles

The PR is ready to go. Making the code xarray ready will be done in a separate PR.

Co-authored-by: RubenImhoff <r.o.imhoff@live.nl>

* weights calculation adjustment outside radar domain if only one model present

* allow for mirroring of advected noise cascade

* implementation of weights following Seed et al. (2013)

* Allow for decomposed NWP precip and NWP velocity fields: part 2

* Store decomposed fields with compression

* changes after first review Daniele

* Remove unnecessary print statement

* fixes to blending utils and implementation of blending utils tests

* remove unnecessary lines

* Fix one time step shift of extrapolation skill prior to blending

* minor changes to blending climatology, blending weights and remove path_workdir from pystepsrc

* Make NWP forecast decomposition prior to blending function optional

* Use pathlib

* Extract methods

* Minor changes to docstrings

* Access climatological skill file for multiple NWP model and date string changes to prevent errors in blending.utils

Co-authored-by: Carlos Velasco <carlos.velasco@bom.gov.au>
Co-authored-by: ned <daniele.nerini@meteoswiss.ch>
Co-authored-by: Andres Perez Hortal <16256571+aperezhortal@users.noreply.github.com>
Co-authored-by: Ruben Imhoff <Ruben.Imhoff@deltares.nl>
Co-authored-by: Carlos Velasco <cvelascof@gmail.com>
Co-authored-by: Lesley De Cruz <lesley.decruz+git@gmail.com>
Co-authored-by: Wout Dewettinck <wout.dewettinck@ugent.be>
Co-authored-by: wdewettin <87696913+wdewettin@users.noreply.github.com>
Co-authored-by: Lesley De Cruz <lesley.decruz@meteo.be>
Co-authored-by: dnerini <daniele.nerini@gmail.com>
@dbkhout dbkhout mentioned this pull request Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants