-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add encode_cf, decode_cf #69
Changes from all commits
88fc6ba
16a927d
d527384
a3cbb16
1ccf00e
a731a76
678977a
6973805
70fddb0
271929a
9de600b
ac7e9e5
2a7cf38
dde34fe
3311a24
7da8e40
0d40b44
818176e
5f0fd82
e3faa52
9d8fa76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ dependencies: | |
# required | ||
- shapely=2 | ||
- xarray | ||
- cf_xarray | ||
# testing | ||
- pytest | ||
- pytest-cov | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,7 +69,7 @@ def multi_dataset(geom_array, geom_array_z): | |
|
||
@pytest.fixture(scope="session") | ||
def multi_geom_dataset(geom_array, geom_array_z): | ||
return ( | ||
ds = ( | ||
xr.Dataset( | ||
coords={ | ||
"geom": geom_array, | ||
|
@@ -80,11 +80,32 @@ def multi_geom_dataset(geom_array, geom_array_z): | |
.set_xindex("geom", GeometryIndex, crs=26915) | ||
.set_xindex("geom_z", GeometryIndex, crs=26915) | ||
) | ||
ds["geom"].attrs["crs"] = ds.xindexes["geom"].crs | ||
ds["geom_z"].attrs["crs"] = ds.xindexes["geom_z"].crs | ||
Comment on lines
+83
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason you can't set these in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apart from "no one thought about that until now", I am not aware of any. |
||
return ds | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
def multi_geom_multi_crs_dataset(geom_array, geom_array_z): | ||
ds = ( | ||
xr.Dataset( | ||
coords={ | ||
"geom": geom_array, | ||
"geom_z": geom_array_z, | ||
} | ||
) | ||
.drop_indexes(["geom", "geom_z"]) | ||
.set_xindex("geom", GeometryIndex, crs=26915) | ||
.set_xindex("geom_z", GeometryIndex, crs="EPSG:4362") | ||
) | ||
ds["geom"].attrs["crs"] = ds.xindexes["geom"].crs | ||
ds["geom_z"].attrs["crs"] = ds.xindexes["geom_z"].crs | ||
return ds | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
def multi_geom_no_index_dataset(geom_array, geom_array_z): | ||
return ( | ||
ds = ( | ||
xr.Dataset( | ||
coords={ | ||
"geom": geom_array, | ||
|
@@ -96,6 +117,9 @@ def multi_geom_no_index_dataset(geom_array, geom_array_z): | |
.set_xindex("geom", GeometryIndex, crs=26915) | ||
.set_xindex("geom_z", GeometryIndex, crs=26915) | ||
) | ||
ds["geom"].attrs["crs"] = ds.xindexes["geom"].crs | ||
ds["geom_z"].attrs["crs"] = ds.xindexes["geom_z"].crs | ||
return ds | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
|
@@ -157,3 +181,18 @@ def traffic_dataset(geom_array): | |
"day": pd.date_range("2023-01-01", periods=10), | ||
}, | ||
).xvec.set_geom_indexes(["origin", "destination"], crs=26915) | ||
|
||
|
||
@pytest.fixture( | ||
params=[ | ||
"first_geom_dataset", | ||
"multi_dataset", | ||
"multi_geom_dataset", | ||
"multi_geom_no_index_dataset", | ||
"multi_geom_multi_crs_dataset", | ||
"traffic_dataset", | ||
], | ||
scope="session", | ||
) | ||
def all_datasets(request): | ||
return request.getfixturevalue(request.param) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the key buggy line. it always sets the index, we do not record which geometry dims were indexed at encode-time. What should we do here?
As an aside it'd be nice for
set_geom_indexes
to understand thegrid_mapping
convention. WDYT?One approach:
decode_cf
does NOT set the new index, but the user does so manually. Insteadset_geom_indexes
learns how to interpret thegrid_mapping
convention so CRS is set properly by default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that an issue if we just index all geom dims encoded in the file?
Not against but I don't really know what would it mean implementation-wise. Maybe just a simple call to
pyproj.CRS.from_cf
?That would be preferable. Not a fan of asking users to set indexes after reading what already was indexed before writing.