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

WIP: use netcdf/cf inspired model for tracking CRS information #899

Merged
merged 43 commits into from
Apr 6, 2020

Conversation

Kirill888
Copy link
Member

@Kirill888 Kirill888 commented Mar 19, 2020

Reason for this pull request

To address issue #837. Rather than rely on xarray attributes that are easily lost use non-dimensional coordinate to capture CRS WKT information.

Proposed changes

  1. Update on-disk representation of ingester generated files
    • mark grid_mapping variable as a coordinate
    • rename grid_mapping variable crs->spatial_ref to not clash with crs attribute
  2. Update dc.load to create xarrays with appropriate structure
  3. Update .geobox code to understand data from 2,3

flake8 complains about starting variable with a Capital letter, for some math
code using variables like X, Y, XX, XY makes perfect sense. since flake8 does
not support per file configuration we just allow a whitelist of such variables
Previously all code assumed that grid_mapping variable is to be called "crs".
With this change making this less hard-coded, it's still not fully configurable
though. The goal is to rename crs -> spatial_ref eventually.
@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #899 into develop will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #899   +/-   ##
========================================
  Coverage    92.70%   92.70%           
========================================
  Files           95       95           
  Lines         9283     9283           
========================================
  Hits          8606     8606           
  Misses         677      677           

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 25b6735...25b6735. Read the comment docs.

test_netcdfwriter. we want to rename this variable, pull it in from netcdf
driver lib instead of assuming it is 'crs'
- expect error when output file exists
- verify that grid_mapping variable is a coordinate and not data var
Store grid_mapping information in a variable named spatial_ref (was crs).

This is because `crs` clashes with `crs` attribute, commonly set on a Dataset.
`.geobox` property is dynamic and is only available when crs is available, so
`dataset.geobox.crs` can never be `None`, since `dataset.geobox` is either None
or a fully qualified `GeoBox` with some non-None `.crs`.
data_resolution_and_offset accept optional fallback_resolution parameter, this
is only used for single element axis.
flake8, we have way too much code that doesn't have spaces around arithmetic
operators, also it's used for paths and `"some_dir"/"file.txt"`, looks more like
a path than `"some_dir" / "file.txt"`
Mapping from pixel space to X/Y space is defined regardless of CRS
Planning to add parameters to it, like whether to include CRS coordinates in the
output or not
Optionally return netcdf/cf style spatial_ref coordinate from xr_coords
Order of resolution is something like:

- x.coords[x.grid_mapping].spatial_ref
- x.coords[<looks like crs coord>].spatial_ref
- x.{x,y,latitude,longitude}.attrs['crs']
- x.attrs['crs']
@Kirill888 Kirill888 marked this pull request as ready for review April 2, 2020 06:12
@Kirill888 Kirill888 requested review from omad and uchchwhash April 2, 2020 06:12
Copy link
Member

@uchchwhash uchchwhash left a comment

Choose a reason for hiding this comment

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

I am OK with this. 👍

datacube/api/core.py Show resolved Hide resolved
datacube/ui/task_app.py Show resolved Hide resolved
@Kirill888 Kirill888 merged commit 9d6e9b4 into develop Apr 6, 2020
@delete-merged-branch delete-merged-branch bot deleted the geobox-netcdf-cf branch April 6, 2020 06:41
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.

Understand NetCDF CF convention for projection info in .geobox property
2 participants