Enable callbacks for loading netcdf with multiple variables #6754
Conversation
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6754 +/- ##
==========================================
- Coverage 90.24% 90.24% -0.01%
==========================================
Files 91 91
Lines 24613 24633 +20
Branches 4604 4612 +8
==========================================
+ Hits 22212 22229 +17
- Misses 1624 1625 +1
- Partials 777 779 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I've suggested some ways to make the code cleaner, and (I think) easier to read.
Also some further tests.
Your earlier suggestion of adding some integration tests is also a good idea, I think.
But that can probably wait while we attend to other things, along with whatsnew content etc.
| break | ||
| return match | ||
|
|
||
| result = inner |
There was a problem hiding this comment.
I really think we should combine these 2 branches,
i.e. the len(constraints) == 1 one and the len(constraints) > 1 one.
Since the code is so similar it totally breaks DRY!
(I suspect you intended to do this + forgot??)
In fact, the new 'branch' code is already covering the ==1 case, since a single constraint is presented as a list of 1 item. I've confirmed that this works.
See code suggestion here.
However, in the process I also found that the len(constraints) > 0 test is essential, because otherwise we return a function which rejects all the vars (!)
Actually, for that reason alone, I think it's worth testing the no-constraints case, which happens not to be tested at present in test__translate_constraints_to_var_callback.py : see separate comment
| ] | ||
| callback = _translate_constraints_to_var_callback(constrs) | ||
| result = [callback(var) for var in self.data_variables] | ||
| self.assertArrayEqual(result, [True, True, False, True, False]) |
There was a problem hiding this comment.
I think it's worth having some extra tests here, which I think cover cases that were not originally checked for (but relevant to the new code).
My suggested additions ...
def test_multiple_constraints__multiname(self):
# Modify the first constraint to require BOTH var-name and std-name match
constrs = [
iris.NameConstraint(standard_name="x_wind", var_name="var1"),
iris.NameConstraint(var_name="var2"),
]
callback = _translate_constraints_to_var_callback(constrs)
# Add 2 extra vars: one passes both name checks, and the other does not
vars = self.data_variables +[
CFDataVariable("var1", MagicMock(standard_name="x_wind")),
CFDataVariable("var1", MagicMock(standard_name="air_pressure"))
]
result = [callback(var) for var in vars]
self.assertArrayEqual(result, [True, True, False, True, False, True, False])
def test_no_constraints(self):
result = _translate_constraints_to_var_callback([])
assert result is None| elif len(constraints) > 1: | ||
| if all( | ||
| isinstance(constraint, iris._constraints.NameConstraint) | ||
| and constraint.STASH == "none" | ||
| for constraint in constraints | ||
| ): | ||
|
|
There was a problem hiding this comment.
From above suggestion to combine 2 code branches.:
My suggestion here ...
constraints = iris._constraints.list_of_constraints(constraints)
if len(constraints) == 0 or not all(
isinstance(constraint, iris._constraints.NameConstraint)
and constraint.STASH == "none"
for constraint in constraints
):
# We can define a var-filtering function to speedup the load, *ONLY* when we
# have some constraints, and all are simple NameConstraints with no STASH.
result = None
else:
def inner(cf_datavar):
. . .N.B. this replaces the initial result=None with an assignment in each side of the 'if'.
| ): | ||
|
|
||
| def inner(cf_datavar): | ||
| match = False |
There was a problem hiding this comment.
I found the use of "match" rather confusing. So here's a bunch of proposals which I think make it easier to read.
See what you think ...
| match = False | |
| match_any_constraint = False |
(I've checked that this works !)
| def inner(cf_datavar): | ||
| match = False | ||
| for constraint in constraints: | ||
| match = True |
There was a problem hiding this comment.
| match = True | |
| match_this_constraint = True |
| continue | ||
| actual = getattr(cf_datavar, attr_name, "") | ||
| if actual != expected: | ||
| match = False |
There was a problem hiding this comment.
| match = False | |
| match_this_constraint = False |
| if match: | ||
| break |
There was a problem hiding this comment.
| if match: | |
| break | |
| if match_this_constraint: | |
| match_any_constraint = True | |
| break |
| break | ||
| if match: | ||
| break | ||
| return match |
There was a problem hiding this comment.
| return match | |
| return match_any_constraint |
3 participants
🚀 Pull Request
Closes #6228