pySTEPS / pysteps

Python framework for short-term ensemble prediction systems.
https://pysteps.github.io/
BSD 3-Clause "New" or "Revised" License
466 stars 168 forks source link

Test steps netcdf exporter #202

Closed dnerini closed 3 years ago

dnerini commented 3 years ago

Closes #200 by including a specific test for the use of a callback function to export a STEPS forecast to netcdf.

Additionally, 42c44d5 fixes a bug introduced by cf34300 which caused an error when a callback was passed with return_output=True.

codecov[bot] commented 3 years ago

Codecov Report

Merging #202 (e7cf0f2) into master (cf34300) will increase coverage by 0.45%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   76.30%   76.75%   +0.45%     
==========================================
  Files         130      130              
  Lines        9690     9716      +26     
==========================================
+ Hits         7394     7458      +64     
+ Misses       2296     2258      -38     
Flag Coverage Δ
unit_tests 76.75% <100.00%> (+0.45%) :arrow_up:

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

Impacted Files Coverage Δ
pysteps/nowcasts/steps.py 86.42% <100.00%> (+1.10%) :arrow_up:
pysteps/tests/test_nowcasts_steps.py 100.00% <100.00%> (+12.00%) :arrow_up:
pysteps/io/nowcast_importers.py 79.38% <0.00%> (+5.15%) :arrow_up:
pysteps/io/exporters.py 52.64% <0.00%> (+7.24%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cf34300...2b2512c. Read the comment docs.

dnerini commented 3 years ago

Tests are currently failing for python 3.8 because of an AttributeError in decorators.py:72:

    @wraps(importer)
    def _import_with_postprocessing(*args, **kwargs):

        precip, *other_args = importer(*args, **kwargs)

        _dtype = kwargs.get("dtype", dtype)

        accepted_precisions = ["float32", "float64", "single", "double"]
        if _dtype not in accepted_precisions:
            raise ValueError(
                "The selected precision does not correspond to a valid value."
                "The accepted values are: " + str(accepted_precisions)
            )

        if isinstance(precip, np.ma.MaskedArray):
            invalid_mask = np.ma.getmaskarray(precip)
            precip.data[invalid_mask] = fillna
        else:
            # If plain numpy arrays are used, the importers should indicate
            # the invalid values with np.nan.
            _fillna = kwargs.get("fillna", fillna)
            if _fillna is not np.nan:
                mask = ~np.isfinite(precip)
                precip[mask] = _fillna

>       return (precip.astype(_dtype),) + tuple(other_args)
E       AttributeError: 'NoneType' object has no attribute 'astype'

/usr/share/miniconda/envs/test-environment/lib/python3.8/site-packages/pysteps/decorators.py:72: AttributeError

@aperezhortal do you have any hint on this?

aperezhortal commented 3 years ago

I was able to reproduce the error in python 3.8. It seems to be originated in the import_netcdf_pysteps importer, on line 206: metadata["threshold"] = np.nanmin(precip[precip > np.nanmin(precip)]). The precip variable has all NaN values, producing the following error:

There was an error processing the file zero-size array to reduction operation fmin which has no identity"

In case of failure, by default, the import_netcdf_pysteps catches the error, print a warning, and returns None, None, None. The first returned None value produced the error on the _import_with_postprocessing decorator since it expects an array for the precipitation field.

Though, I still couldn't find why this error is only raised in python 3.8 and not in 3.6.

I'll dig a little bit more into it.

RubenImhoff commented 3 years ago

Nice work! I'll try to take a look at it today. :)

dnerini commented 3 years ago

I was able to reproduce the error in python 3.8. It seems to be originated in the import_netcdf_pysteps importer, on line 206: metadata["threshold"] = np.nanmin(precip[precip > np.nanmin(precip)]). The precip variable has all NaN values, producing the following error:

There was an error processing the file zero-size array to reduction operation fmin which has no identity"

In case of failure, by default, the import_netcdf_pysteps catches the error, print a warning, and returns None, None, None. The first returned None value produced the error on the _import_with_postprocessing decorator since it expects an array for the precipitation field.

Though, I still couldn't find why this error is only raised in python 3.8 and not in 3.6.

I'll dig a little bit more into it.

Thanks Andres for having a look. Unfortunatly I can't manage to reproduce the error in my 3.8 environment. Can you please share the steps you used?

aperezhortal commented 3 years ago

These are the steps I followed:

conda create -n test_py38 python=3.8

conda activate test_py38

conda install cython numpy jsmin jsonschema matplotlib netCDF4 opencv pillow pyproj pygrib scipy dask pyfftw cartopy h5py PyWavelets pandas scikit-image pytest pytest-cov

pip install git+https://github.com/pySTEPS/pysteps.git@test-netcdf-exporter

Then, from from the tests directory:

pytest test_nowcasts_steps.py

The error seems to related to the new version of scipy (1.6.0) that introduced some improvements to scipy.ndimage (see here). If scipy is downgraded to v1.4 (used for the python 3.6 env in conda), the tests pass.

dnerini commented 3 years ago

Thanks @aperezhortal for the help, I was able to reproduce the error by following your steps.

This looks a bit complicated and probably deserves a separate issue. For the moment I was able to avoid the error by setting vel_pert_method=None instead of the default vel_pert_method="bps" when calling the nowcasts.steps (see ef6288f). From what I understand, the AttributeError mentioned above is caused by

  1. output of steps.forecast being all NaNs (this is what @aperezhortal noticed in the first place) with scipy>=1.6.
  2. which is caused by the call to extrapolator_method(R_f_ip, V_pert, [t_diff_prev], **extrap_kwargs_) (L#757 of steps.py) (Note that this is calling semilagrangian.extrapolate )
  3. which caused by the variable V_pert being all NaNs
  4. which is returned by generate_vel_noise(vps[j], t_total[j] * timestep) (L#754 in steps.py)
  5. which is caused by lines N = linalg.norm(V, axis=0) and V_n = V / np.stack([N, N]) (L#127-128 in noise/motion.py) when variable V is all zeros (as in my test case, since I set the motion field to zero).

Worth noticing that with scipy<1.6, step 2 seems to return a field of finite values instead, which doesn't trigger the final AttributeError. I need to look into this a bit more, but it seems that the behavior of semilagrangian.extrapolate with respect to input NaNs changed. Maybe related to recent changes in map_coordinates? @pulkkins