Nowcast refactoring

[pulkkins]

This pull request contains major refactoring and polishing of the nowcasts module.

Refactoring

• Implement a generic utility function (nowcasts.utils.nowcast_main_loop) for iterative computation of nowcasts. The function supports adding perturbations and using parallelization, callback function or non-integer time steps.
• Rewrite the nowcasting methods using the above function to reduce the amount of redundant code.
• S-PROG and STEPS: use a common utility function for computing the wet area ratio and incremental masks to reduce redundant code.
• LINDA: implement support for non-integer time steps and the callback and return_output options as in STEPS.
• Add callback function tests for LINDA.

Minor fixes

• S-PROG and STEPS: ensure that the user specifies the rain rate threshold when it's required
• Use f-strings instead of old-style printing.
• Use consistent PEP 8-compatible naming for the inputs of the nowcasting methods (e.g. precip and velocity instead of R and V).
• Other variable name changes to enhance PEP 8-compatibility.
• Ensemble nowcasts: make sure that dask is used only when ensemble size is greater than one and do not try to allocate more workers than ensemble members.
• Fix typos and polish docstrings.
• Use consistent docstring indentation (4 spaces).

[dnerini]

hey @pulkkins, I just tried to build the documentation, which failed (logs here) because we renamed many positional arguments in the nowcasting methods...

These represent breaking changes (and indeed, they broke our documentation ...). Since most of them are only "aesthetic changes" (naming conventions), I wonder whether they're justified.

So what do we do? Should we just revert those changes (when they concern input arguments) and keep only renaming of internal variables?

[mathause]

Another possibility is to deprecate the old names in favor of the new ones - this can e.g. be achieved as:

https://github.com/regionmask/regionmask/blob/e5416cc03027c69a0b1a451860c4028d43dadc21/regionmask/core/plot.py#L225-L230

I would also consider making most of the arguments keyword-only e.g. def f(a, *, b=5), this will force the user to write everything out, making it more understandable in the long term. This could be hard-deprecated with:

def func(a, b, c, *args, d=5, e="param"):

    if args:
        raise TypeError("Only three positional arguments allowed")

---

For context @leabeusch told me to have a look at pysteps so I did ;-)

[dnerini]

Hello @mathause and welcome to pysteps! Many thanks for your comments, see my answers below.

Another possibility is to deprecate the old names in favor of the new ones - this can e.g. be achieved as:

https://github.com/regionmask/regionmask/blob/e5416cc03027c69a0b1a451860c4028d43dadc21/regionmask/core/plot.py#L225-L230

Very good point, yes, we could raise a future deprecation warning for the old argument names. I think I'll give this a try, if you don't mind @pulkkins ?

edit: done in c81c848, any feedback is appreciated!

I would also consider making most of the arguments keyword-only e.g. def f(a, *, b=5), this will force the user to write everything out, making it more understandable in the long term. This could be hard-deprecated with:

def func(a, b, c, *args, d=5, e="param"):



    if args:

        raise TypeError("Only three positional arguments allowed")

I think we loosely follow this convention in most of our methods (I should check...), but I like the idea of enforcing it. Maybe material for a separate issue?

For context @leabeusch told me to have a look at pysteps so I did ;-)

Then thanks @leabeusch for sending you this way! 😄

[pulkkins]

I have not yet done extensive refactoring of the nowcasts.sseps module. @dnerini would you like to work on that or should we merge this pull request?

[dnerini]

I have not yet done extensive refactoring of the nowcasts.sseps module. @dnerini would you like to work on that or should we merge this pull request?

No I think that should not be in scope for this PR. Anyway, it is my intention to deprecate sseps (for sure in v2), since linda provides a better implementation of the localisation mechanism.

so yes, please go ahead and merge this PR!