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

Ide nowcasting #221

Merged
merged 120 commits into from
Aug 7, 2021
Merged

Ide nowcasting #221

merged 120 commits into from
Aug 7, 2021

Conversation

pulkkins
Copy link
Member

@pulkkins pulkkins commented Jul 27, 2021

This branch implements the Lagrangian INtegro-Difference equation model with Autoregression (LINDA) developed by Pulkkinen et al. (2021).

Remaining issues that will be addressed in the next version:

  • The timesteps argument is required to be an integer. Supplying the nowcast timesteps in a list is not yet supported.
  • There are several functions that overlap with existing methods implemented in pysteps. For instance, _estimate_ar*_params, _iterate_ar_model and _window_tukey can be replaced with existing methods or moved outside linda.py.

@pulkkins
Copy link
Member Author

pulkkins commented Aug 1, 2021

I noticed some duplication in the code, for example you reimplemented the Tukey tapering window even though we already have it available in the utils.tapering module. Probably not something urgent to be done now, but I would strongly recommend to try to reduce code duplication by using methods already available in pysteps. If for some reasons the existing methods do not cover your needs, we should first try to extend or modify them instead of creating new ones (especially now that V2 #216 will give us more freedom for breaking changes). Otherwise, the risk is to get to a point of having a code base that is impossible to maintain...

What do you think, @pulkkins ? Is there something we can do now or is it better to close this PR and keep this in mind for a small refactoring in V2?

Let's do that in V2. The purpose of this pull request is to implement the first usable version of LINDA. It can be refined later.

pysteps/nowcasts/linda.py Outdated Show resolved Hide resolved
@dnerini
Copy link
Member

dnerini commented Aug 3, 2021

With the changes introduced in #225, were you thinking of adding a default max_num_features ? Something around 20?

@pulkkins
Copy link
Member Author

pulkkins commented Aug 3, 2021

With the changes introduced in #225, were you thinking of adding a default max_num_features ? Something around 20?

Done.

@dnerini
Copy link
Member

dnerini commented Aug 4, 2021

Very nice, well done @pulkkins ! If you agree, id like to do one last profiling with the default parameters and, if everything looks good, I think we can merge.

Edit: here it is for the same case as above (640x710 domain, 12 time steps).

Sequential Parallel
Parameters 25 blobs, 10 members, 1 worker 25 blobs, 10 members, 10 workers
image image
Total time (forecast loop) 11 min (ca. 4 seconds / member / time step) 4 min (ca. 0.5 seconds / member / time step)
Peak memory 1.2 GB 4.2 GB

@dnerini
Copy link
Member

dnerini commented Aug 6, 2021

The RTD built for this branch can be found here:
https://pysteps.readthedocs.io/en/ide_nowcasting/

@pulkkins pulkkins merged commit 934028a into master Aug 7, 2021
@dnerini dnerini deleted the ide_nowcasting branch August 9, 2021 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants