Closed RubenImhoff closed 3 years ago
Thanks for your useful feedback, @dnerini! I didn't have time to work on it this week, but the changes will follow asap. :)
@cvelascof and @ladc, see also the description in issue #215, I have made some changes to the nwp importers to set the cell coordinates to the center of the cells. Could you check if this is also the center for the rmi and bom importers?
So far, I've sticked to rasterio instead of rioxarray. When trying rio, I kept running into projection issues. Maybe it is just me, or rio is not as stable yet as rasterio. Feel free to try it out and adjust the scripts, if you know how to do it properly!
I do agree with @dnerini that it would be nice to eventually move to rioxarray.
Tests still fail due to rasterio, I haven't figured out yet why..
the sample NWP datasets used for BoM are already regrided to the same projection of the BoM radar, so coordinates are at the center of the pixel
Except for the use of rioxarray instead of rasterio, I think this PR is getting ready for a final review. We may implement rioxarray later, unless someone has enough experience with it to make it compatible for a variety of spatial projections in our radar and nwp products. Only the tests still fail, due to the import of the rasterio module. Any idea what causes this?
Only the tests still fail, due to the import of the rasterio module. Any idea what causes this?
mmmh you sure it is linked to the import? I see a RuntimeError when you load the values from the netcdf at line 87 in reprojection.py. My guess is that this creates too many new threads (because of the loop?). Things to try: load the data before the loop, try to use xarray's apply_ufunc instead.
So far, I've sticked to rasterio instead of rioxarray. When trying rio, I kept running into projection issues. Maybe it is just me, or rio is not as stable yet as rasterio.
Thanks for trying it out! I'm really fine with sticking to rasterio if it simplifies things to us. As you say, we can alwas decide to migrate to rioxarray later on.
Thanks a lot for looking into this, @dnerini! I'll make the changes coming Monday. :)
Hmm, I keep getting "pysteps.exceptions.MissingOptionalDependency: rasterio package is required for the reprojection tool, but it isnot installed". What am I doing wrong in the development dependencies? It is listed as an optional dependency.
maybe there is a problem with the installation of rasterio in the test environment? Try to remove the try... except block to expose directly the original error message
Good call, that changes it in "ModuleNotFoundError: No module named 'rasterio'" unfortunately. I think I've missed an optional dependency or so for the tester, but can't find it :(
Sorry I didn't notice that rasterio is missing from the test dependencies , look at the top of .github/workflows/test_pysteps.yaml
Timing, just found it myself too. It had to be somewhere. ;) Thanks for the help!
Merging #236 (26e51a9) into pysteps-v2 (4d1fb51) will increase coverage by
0.16%
. The diff coverage is96.38%
.
@@ Coverage Diff @@
## pysteps-v2 #236 +/- ##
==============================================
+ Coverage 73.67% 73.84% +0.16%
==============================================
Files 146 148 +2
Lines 11056 11128 +72
==============================================
+ Hits 8146 8217 +71
- Misses 2910 2911 +1
Flag | Coverage Δ | |
---|---|---|
unit_tests | 73.84% <96.38%> (+0.16%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
pysteps/tests/test_io_bom_nwp.py | 100.00% <ø> (ø) |
|
pysteps/tests/test_io_knmi_hdf5.py | 100.00% <ø> (ø) |
|
pysteps/tests/test_io_knmi_nwp.py | 100.00% <ø> (ø) |
|
pysteps/tests/test_io_rmi_nwp.py | 100.00% <ø> (ø) |
|
pysteps/utils/reprojection.py | 91.89% <91.89%> (ø) |
|
pysteps/io/importers.py | 73.92% <100.00%> (ø) |
|
pysteps/io/nwp_importers.py | 90.04% <100.00%> (+0.41%) |
:arrow_up: |
pysteps/tests/test_utils_reprojection.py | 100.00% <100.00%> (ø) |
|
pysteps/utils/__init__.py | 100.00% <100.00%> (ø) |
|
pysteps/utils/interface.py | 87.69% <100.00%> (+0.39%) |
:arrow_up: |
... and 2 more |
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 4d1fb51...26e51a9. Read the comment docs.
Tests succeed now! Only the tests for pysteps 3.6 fail due to rasterio looking for the gdal_version, which gives an error in only pysteps 3.6 for windows. Any ideas on how to fix this? Other than that, the PR is probably good to go. A recommended change for later is to move from rasterio to rioxarray.
Tests succeed now! Only the tests for pysteps 3.6 fail due to rasterio looking for the gdal_version, which gives an error in only pysteps 3.6 for windows. Any ideas on how to fix this? Other than that, the PR is probably good to go. A recommended change for later is to move from rasterio to rioxarray.
GDAL can be a real pain to install, but I thought conda would help in that sense. Maybe you can check that the right version of GDAL is installed?
Besides the error, your tests in test_utils_reprojection.py
cannot pass if rasterio is not installed, which can happen as it is an optional dependency.
please add pytest.importorskip("rasterio")
at the top of the test, and do the same for any other optional dependency in there.
Thanks to @wdewettin for most of the work! This PR resolves issue #230.
Reprojection functionality for changing the projection and grid (up/downscaling possible) of input fields - generally NWP data for blending - with a destination field (the original radar field for instance). This functionality was not yet present in pysteps, but becomes necessary for blending two different data sources. Note that this implementation uses a new module, rasterio, which we may have to make an optional dependency in pysteps. It is worth reconsidering this, but at least the script is working and stable now with both the Belgian and Dutch data.