-
Notifications
You must be signed in to change notification settings - Fork 55
Zonal mpsi function #773
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
base: main
Are you sure you want to change the base?
Zonal mpsi function #773
Changes from all commits
4c01f3f
45256b4
c70cf22
92d121c
383443d
ac52e9f
b68952a
66fa9a7
f28b382
8003b7a
3db32b7
8747e81
84e191d
bb53840
f73f9ad
8226fa3
ac66118
19c6827
5862a3f
f6ce027
0d44645
54906c9
18dd80c
728337f
51b15ee
7408af9
d582684
ccd117b
886412b
1bd1b0d
275b3ad
0e13cf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,3 +18,4 @@ dependencies: | |
| - nbsphinx | ||
| - xskillscore>=0.0.17 | ||
| - nc-time-axis | ||
| - uxarray | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,3 +23,4 @@ dependencies: | |
| - pytest | ||
| - pytest-cov | ||
| - geocat-datafiles | ||
| - uxarray | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||
| import typing | ||||||
| import warnings | ||||||
|
|
||||||
|
|
||||||
| def _generate_wrapper_docstring( | ||||||
|
|
@@ -27,3 +28,128 @@ def _generate_wrapper_docstring( | |||||
|
|
||||||
| # assign docstring to wrapper function | ||||||
| setattr(wrapper_fcn, '__doc__', wrapper_docstring) | ||||||
|
|
||||||
|
|
||||||
| def _find_var( | ||||||
| ds, | ||||||
| standard_name=None, | ||||||
| long_name=None, | ||||||
| possible_names=None, | ||||||
| units=None, | ||||||
| description='variable', | ||||||
| ): | ||||||
| """ | ||||||
| Find a variable using CF-compliant checks. | ||||||
|
|
||||||
| Searches in priority order: | ||||||
| 1. CF standard_name attribute match (if standard_names provided) | ||||||
| 2. Name attribute match (if long_names provided) | ||||||
| 3. Direct variable name match (if possible_names provided) | ||||||
| 4. Units match (if units provided) | ||||||
|
|
||||||
| Parameters | ||||||
| ---------- | ||||||
| ds : xr.Dataset or ux.UxDataset | ||||||
| The dataset to search | ||||||
| standard_name : str, optional | ||||||
| CF standard_name to check in attrs | ||||||
| long_name : str, optional | ||||||
| long_name to check in attrs | ||||||
| possible_names : list of str, optional | ||||||
| List of possible variable names to check first | ||||||
| units : str, optional | ||||||
| Possible units to check in attrs | ||||||
| description : str, optional | ||||||
| String | ||||||
|
|
||||||
| Returns | ||||||
| ------- | ||||||
| str | ||||||
| The name of the found variable | ||||||
|
|
||||||
| Raises | ||||||
| ------ | ||||||
| KeyError | ||||||
| If no matching variable is found | ||||||
| """ | ||||||
| error_parts = [] | ||||||
|
|
||||||
| # First try CF standard_name attribute match | ||||||
| if standard_name: | ||||||
| for var_name in ds.data_vars: | ||||||
| var_attrs = ds[var_name].attrs | ||||||
| if var_name == standard_name or standard_name in var_attrs.get( | ||||||
| 'standard_name', '' | ||||||
| ): | ||||||
|
Comment on lines
+81
to
+83
Member
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. (for this and all the following) I feel like we should make this at least a little more robust, like
|
||||||
| return var_name | ||||||
| error_parts.append(f"Tried standard_name: {standard_name}. ") | ||||||
|
|
||||||
| # Then try long_name attribute match | ||||||
| if long_name: | ||||||
| for var_name in ds.data_vars: | ||||||
| var_attrs = ds[var_name].attrs | ||||||
| if long_name in var_attrs.get('long_name', ''): | ||||||
| return var_name | ||||||
| error_parts.append(f"Tried long_name: {long_name}. ") | ||||||
|
|
||||||
| # Then try direct name match | ||||||
| if possible_names: | ||||||
| for name in possible_names: | ||||||
| if name in ds: | ||||||
| return name | ||||||
| error_parts.append(f"Tried names: {possible_names}. ") | ||||||
|
|
||||||
| # Finally try units match (less reliable) | ||||||
|
Member
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. Using units here still concerns me. I'm not convinced that units are a good way of determining a match. |
||||||
| if units: | ||||||
| for var_name in ds.data_vars: | ||||||
| var_attrs = ds[var_name].attrs | ||||||
| if 'units' in var_attrs: | ||||||
| if units in var_attrs.get('units', ''): | ||||||
| warnings.warn( | ||||||
| f"Found {description} '{var_name}' using units attribute only. " | ||||||
| f"This is unreliable - multiple variables may share the same units. " | ||||||
| f"Please verify this is correct and add CF standard_name. {error_parts}", | ||||||
| UserWarning, | ||||||
| stacklevel=3, | ||||||
|
Member
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. Why |
||||||
| ) | ||||||
| return var_name | ||||||
| error_parts.append(f"Tried units: {units}. ") | ||||||
|
|
||||||
| raise KeyError(f"Could not find {description} in dataset. {' '.join(error_parts)}") | ||||||
|
Member
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.
Suggested change
|
||||||
|
|
||||||
|
|
||||||
| def _find_optional_var( | ||||||
| ds, | ||||||
| standard_name=None, | ||||||
| long_name=None, | ||||||
| possible_names=None, | ||||||
| units=None, | ||||||
| description=None, | ||||||
| ): | ||||||
| """ | ||||||
| Find an optional variable using CF-compliant checks. | ||||||
|
|
||||||
| Parameters | ||||||
| ---------- | ||||||
| ds : xr.Dataset or ux.UxDataset | ||||||
| The dataset to search | ||||||
| standard_name : str, optional | ||||||
| CF standard_name to check in attrs | ||||||
| long_name : str, optional | ||||||
| Long_name to check in attrs | ||||||
| possible_names : list of str, optional | ||||||
| List of possible variable names | ||||||
| units : str, optional | ||||||
| Possible units to check in attrs | ||||||
|
|
||||||
| Returns | ||||||
| ------- | ||||||
| str or None | ||||||
| The name of the found coordinate, or None if not found | ||||||
| """ | ||||||
| try: | ||||||
| return _find_var( | ||||||
| ds, standard_name, long_name, possible_names, units, description | ||||||
| ) | ||||||
| except KeyError: | ||||||
| return None | ||||||
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'd say yes. There were updates to UXarray's where and some wrappers that were previously roadblocks to this function. Perhaps conda will naturally resolve the environment to the latest version, but I'd like to let more time pass before trusting that.