-
Notifications
You must be signed in to change notification settings - Fork 287
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
reuploading changes for bounds of derived variables #6350
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6350 +/- ##
==========================================
- Coverage 89.79% 89.60% -0.20%
==========================================
Files 90 90
Lines 23554 23608 +54
Branches 4391 4411 +20
==========================================
+ Hits 21150 21153 +3
- Misses 1662 1710 +48
- Partials 742 745 +3 ☔ View full report in Codecov by Sentry. |
Note (for my benefit!) This comment is a useful summary of the agreed behaviour we should be delivering |
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.
Just a couple notes on places to improve the code. As mentioned offline, the major thing this needs now tests. It's worth pushing something up demonstrating expected behaviour even if they're currently failing.
cf_root_var = self.cf_group[cf_root] | ||
if not hasattr(cf_root_var, "standard_name"): | ||
continue | ||
name = cf_root_var.standard_name or cf_root_var.long_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.
if you reach this point you will have a standard name, so or cf_root_var.long_name
is unnecessary.
cf_root_coord = self.cf_group.coordinates.get(cf_root) | ||
if cf_root_coord is None: | ||
cf_root_coord = self.cf_group.auxiliary_coordinates.get(cf_root) | ||
with contextlib.suppress(AttributeError): |
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 feel like this is probably not necessary. Would you not want to do something like bounds_name = getattr(cf_root_coord, "bounds", default=None)
?
bounds_name = cf_root_coord.bounds | ||
|
||
if bounds_name is not None: | ||
try: |
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.
Ultimately, I think you'll probably want to replace this try-except call. I expect you ought to be able to replace this with something like:
bounds_vars = [...]
if len(bounds_vars) == 1:
bounds_var = bounds_vars[0]
...
else:
...
🚀 Pull Request
Description
I seem to have royally screwed up the old PR for this. So here are all of the relevant current changes for that.
Will copy over all of the discussions from the PR soon.
Consult Iris pull request check list
Add any of the below labels to trigger actions on this PR: