fix zarr convention - #260
Conversation
keewis
left a comment
keewis
left a comment
There was a problem hiding this comment.
thanks for the PR, @carloshorn!
Looks good to me, except I'd move the treatment of the missing coordinate to the specific index implementation (e.g. HealpixIndex). Which feels like it's going to be quite a bit of work, so you may want to move that to a separate PR.
|
Before fixing the merge conflict. What do you think about this change? |
There was a problem hiding this comment.
sounds good to me in general. Just a caveat, I've almost finished supporting compression types in the zarr convention, so there might be more merge conflicts soon (but also this may simplify a lot of the code here). In particular, though, I just realized I also have a fix for the ellipsoid naming.
I'm not sure how I'll manage these differences, but if you're game we can try to merge this first and then I'll resolve the merge conflicts in my PR.
Edit: see #262
So yes, I'm pretty happy with the direction of this PR, I just have a few small suggestions after a quick first pass.
| elif indexing_scheme == "nuniq": | ||
| start = 4 ** (1 + level) | ||
| stop = start + size | ||
| cell_ids = xr.indexes.PandasIndex( | ||
| pd.RangeIndex(start, stop, name=name), dim | ||
| ) |
There was a problem hiding this comment.
I don't think we support "nuniq" in healpix-geo yet, so naturally this will fail for now (but possibly we can keep it commented out)
| return cls(var.data, dim, name, grid_info, index_kind=index_kind) | ||
|
|
||
| @classmethod | ||
| def from_level( |
There was a problem hiding this comment.
maybe call this full_domain (or at least, that's what I've called this for the RangeMOCIndex)?
This will also need to account for index_kind="moc", which I believe should just forward to the moc index and RangeMOCIndex.full_domain.
|
|
||
| @classmethod | ||
| def from_level( | ||
| cls: type[DGGSIndex], |
There was a problem hiding this comment.
according to https://typing.python.org/en/latest/spec/annotations.html#annotating-instance-and-class-methods you don't need to annotate cls, or if you do you can annotate it as type[Self] (and annotate the return type of the class method as Self)
|
I've decided to instead merge #262 first and adapt here. To make this easier on you I've resolved the merge conflicts for you. This does mean however that this PR does not need to translate the ellipsoid parameter names anymore, but the additional test coverage is definitely nice (but it looks like we'll need even more tests, which I'll add in a separate PR). |
|
Thanks for the initiative @keewis. I will look into it early next week. |
3 participants
changelog.mdapi.rstI went through the Zarr DGGS spec and introduced few changes which I also covered with tests.
semi_major_axisandsemi_minor_axisis now translated correctly.refinement_levelis now checked.