-
Notifications
You must be signed in to change notification settings - Fork 6
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
Default to CF standard axes for accessing dimension names #20
Conversation
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.
Thanks, looks good.
One comment on handling the failures.
Can you also add a test? I'd want to make sure that users can override things. Maybe something silly like x_dimension="time"
and check the result.
xstac/_xstac.py
Outdated
if kw is None: | ||
try: | ||
kw = ds.cf[CF_STANDARD_AXES[kw_name]].name | ||
except KeyError: |
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.
I'm not sure how to handle this case. In general, I'd assume people don't look at logs and so just logging the warning isn't great, and might just annoy people who don't want that dimension.
How about this: the default of None
will mean get it from the CF conventions by default, but we just raise this KeyError if it's not present for time
, y
, and x
.
Users who have those dims, but don't / can't use CF conventions can specify time="my-time-dim"
.
Users who don't have those dims can drop them explicitly with time=False
.
So I think the whole try / except
can be removed, or you could use it to reraise with a better error message
try:
...
except KeyError as e:
raise KeyError("better message about setting {dim}=False to drop") from e
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.
Added two descriptive errors in c3e1001
Just saw this:
I think #19 (comment) might have been mistaken about the behavior of |
Added two error conditions to the cf handler func, and tests for both. Also added testing for explicit dimension name passing via the In working on the tests, I found the single
This remains the one untested (untestable?) aspect. This hypothetical My understanding is that STAC objects require a temporal dimension. If so, what would be the use case for |
Changed recommendation in 78dd461 to omit suggesting |
xstac/_xstac.py
Outdated
) | ||
if not hasattr(ds, "cf"): | ||
raise AttributeError( | ||
f"Kwarg `{kw_name}` is None and the dataset does not have a `cf` namespace. Please " |
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 this error message right? I would expect that importing cf_xarray
here would ensure that the accessor is available, regardless of whether it was imported when the Dataset was created.
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 this error message right?
No. Removed in b1b4004
I would expect that importing cf_xarray here would ensure that the accessor is available
Correct. I hadn't realized this. And I think removing the cf_xarray
import in conftest.py
(where the test datasets are created) serves as a de-facto test/verification of this expectation: b1b4004#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128
Great, thanks @cisaacstern! |
A continuation of #19, which I accidentally closed.
This PR achieves the recommendation made in #19 (comment) (with the exception of the reference system part), in that
now yields the same result as
because the correct values for the x and y dimensions are inferred from the CF namespace in
which is called on each dimension kwarg in
xarray_to_stac
.The logger warning is my attempt to make it clear to the user what's happening if the key is missing; we don't want to raise an error because there may be situations where dimensions are intentionally omitted, as noted in #19 (comment).
I'm using the axes names instead of the standard names, because I understand them to be more generalizable than "latitude" and "longitude". The axes-based approach does work both for the existing tests as well as the Terraclimate example. If we do want to go this route, I could build in some additional testing for explicit dimension name passing as well as the
KeyError
edge case.