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

t.rast.import.netcdf: handle empty CRS and no subdataset #1188

Merged
merged 14 commits into from
Sep 4, 2024

Conversation

ninsbl
Copy link
Member

@ninsbl ninsbl commented Aug 25, 2024

This PR address various issues:

  • NC files without CRS are now treated as CRS84 (default in GDAL)
  • NC files without subdatasets were not handled properly
    In addition, several formating issues were addressed...

@ninsbl ninsbl requested a review from veroandreo August 25, 2024 10:35
@ninsbl ninsbl linked an issue Aug 25, 2024 that may be closed by this pull request
@ninsbl ninsbl added bug Something isn't working Python Related code is in Python labels Aug 25, 2024
@ninsbl ninsbl self-assigned this Aug 25, 2024
@veroandreo
Copy link
Contributor

I just tested with the same command reported in #1182 and I get:

r.in.gdal input=/vsicurl/https://data.chc.ucsb.edu/products/CHIRPS-2.0/global_daily/netcdf/p05/chirps-v2.0.2011.days_p05.nc band=7 memory=300 offset=0 num_digits=0 output=chirps_v2_0_2011_days_p05_20110107T000000.convective_precipitation_rate -a --q
ERROR: Coordinate reference system of dataset does not appear to match
       current project.

       Project PROJ_INFO is:
       name: Lat/Lon
       proj: ll
       datum: wgs84
       ellps: wgs84
       init: EPSG:4326

       Dataset PROJ_INFO is:
       Dataset proj = 0 (unreferenced/unknown)

       Difference in: proj

       In case of no significant differences in the CRS definitions, use
       the -o flag to ignore them and use current project definition.
       Consider generating a new project from the input dataset using the
       'project' parameter.
Exception in thread Thread-7 (_handle_results):
Traceback (most recent call last):
  File "/usr/lib/python3.11/threading.py", line 1045, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.11/threading.py", line 982, in run
    self._target(*self._args, **self._kwargs)
  File "/usr/lib/python3.11/multiprocessing/pool.py", line 579, in _handle_results
    task = get()
           ^^^^^
  File "/usr/lib/python3.11/multiprocessing/connection.py", line 251, in recv
    return _ForkingPickler.loads(buf.getbuffer())
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: CalledModuleError.__init__() missing 3 required positional arguments: 'module', 'code', and 'returncode'

and I need to Ctrl-C to stop it. I am not sure this is an issue of t.rast.import.netcdf or r.in.gdal. If I add the -o flag to override proj check, it goes fine.

@ninsbl
Copy link
Member Author

ninsbl commented Aug 28, 2024

Sorry, @veroandreo The CHIRPS dataset was a bit tricky. The upside is that it exposed a few other bugs in corner cases...

Now I added a test for CHIRPS as well and extended the testsuite.

That particular dataset may still cause trouble in praxis as the server seems very slow. It could be an advantage to download and import (or link) from there. That said, users must take care if reprojection is required on import (no -o flag). In that case the computational region has to be defined reasonably and the r-flag should probably be set (esp. when importing global data into projected CRS).... Added a few sentence on that in the manual...

@ninsbl
Copy link
Member Author

ninsbl commented Aug 29, 2024

Tests are failing due to version conflict in cf_units. It does not yet support NumPy 2, while CI uses NumPy 2.0. @echoix I saw you commented on a related issue in the cf_units repo. Should we try installing it from source in CI for the time beeing? The development version should support NumPy 2 and cf_units is only used in t.rast.import.netcdf as far as I can see...

@echoix
Copy link
Member

echoix commented Aug 29, 2024

Tests are failing due to version conflict in cf_units. It does not yet support NumPy 2, while CI uses NumPy 2.0. @echoix I saw you commented on a related issue in the cf_units repo. Should we try installing it from source in CI for the time beeing? The development version should support NumPy 2 and cf_units is only used in t.rast.import.netcdf as far as I can see...

There might be hope, but we shouldn't count on this SciTools/cf-units#438 (comment)

@ninsbl
Copy link
Member Author

ninsbl commented Sep 4, 2024

Tests pass locally. Is it OK to merge now and wait for cf_units to be updated? Then the improved / working module would become available. Tests should pass as soon as cf_units is NumPy 2 compatible... Will check that. We use the module in praxis and actually need the fix...

@ninsbl
Copy link
Member Author

ninsbl commented Sep 4, 2024

Taking the liberty to merge with tests failing due to version conflict in CI setup. Having the module working is needed. Tests will succeed when cf_units supports NumPy 2...

@ninsbl ninsbl merged commit c870a90 into OSGeo:grass8 Sep 4, 2024
7 checks passed
@ninsbl ninsbl deleted the t_netcdf branch September 4, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] t.rast.import.netcdf: Dataset object is not subscriptable
3 participants