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

Test steps netcdf exporter #202

Merged
merged 6 commits into from
Jan 13, 2021
Merged

Test steps netcdf exporter #202

merged 6 commits into from
Jan 13, 2021

Conversation

dnerini
Copy link
Member

@dnerini dnerini commented Jan 7, 2021

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.

@dnerini dnerini added this to the release v1.4.1 milestone Jan 7, 2021
@codecov
Copy link

codecov bot commented Jan 7, 2021

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%) ⬆️

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%) ⬆️
pysteps/tests/test_nowcasts_steps.py 100.00% <100.00%> (+12.00%) ⬆️
pysteps/io/nowcast_importers.py 79.38% <0.00%> (+5.15%) ⬆️
pysteps/io/exporters.py 52.64% <0.00%> (+7.24%) ⬆️

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
Copy link
Member Author

dnerini commented Jan 7, 2021

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
Copy link
Member

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
Copy link
Contributor

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

Copy link
Contributor

@RubenImhoff RubenImhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, @dnerini! This tester would find the issue in the future.

@dnerini
Copy link
Member Author

dnerini commented Jan 8, 2021

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
Copy link
Member

aperezhortal commented Jan 9, 2021

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.

Turns out that vel_pert_method='bps' would return all NaNs with a zero motion field (python=3.8)
@dnerini
Copy link
Member Author

dnerini commented Jan 9, 2021

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

@dnerini dnerini merged commit 2a5f7fa into master Jan 13, 2021
@dnerini dnerini deleted the test-netcdf-exporter branch January 15, 2021 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

netCDF exporter does not contain any time steps
4 participants