-
Notifications
You must be signed in to change notification settings - Fork 41
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
Assign new variables by standard names #516
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
I think the cf-xarray version of this only really makes sense when the assigned name is a standard name on one of the present variables, so (2) in your listing. For (1), we should just forward on to Xarray, as usual.
Yes, this is intentional. |
# for each standard name and value pair | ||
for standard_name, values in standard_variables.items(): | ||
# default to using existing short name or standard name | ||
name = getattr(values, "name", standard_name) |
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.
Something like
cf-xarray/cf_xarray/accessor.py
Line 2473 in 52160a5
apply_mapper(_get_all, self._obj, key, error=False, default=[key]) |
should be used here. So
mapped_name, = apply_mapper(_get_all, self._obj, standard_name, error=False, default=[standard_name])
will do the translation to actual name in the dataset, if possible. Use this to create a new dictionary that you can then pass to self._obj.assign
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.
Thank you for the feedback, I will be looking at this next week.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #516 +/- ##
==========================================
- Coverage 85.78% 85.39% -0.39%
==========================================
Files 13 13
Lines 2364 2623 +259
Branches 183 241 +58
==========================================
+ Hits 2028 2240 +212
- Misses 303 341 +38
- Partials 33 42 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This pull request implements
ds.cf.assign(**standard_variables)
. The new method should:**kwargs
or dictionary.Also needed:
I would welcome opinions on the two points below.
1. Decide about the variable short name
I propose this algorithm:
2. When dataset already contains standard name
I find it more difficult to decide what the method should do here.
Dataset.assign
).existing: ignore, override, raise, warn
Note: if we allow multiple variables with the same standard name, the resulting Dataset is technically valid, and
ds.cf
shows several variables associated with one standard name, whileds.cf[standard_name] fails with a
KeyError`.