Redirect ALL un-loadable NetCDF objects to LOAD_PROBLEMS
#6465
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6465 +/- ##
==========================================
+ Coverage 89.80% 89.86% +0.05%
==========================================
Files 90 90
Lines 23755 23866 +111
Branches 4418 4434 +16
==========================================
+ Hits 21334 21447 +113
+ Misses 1672 1669 -3
- Partials 749 750 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -48,7 +48,7 @@ | |||
| <dimCoord id="62e940e0" points="[10, 20, 30]" shape="(3,)" standard_name="longitude" units="Unit('degrees')" value_type="int32" var_name="lon"/> | |||
| </coord> | |||
| <coord datadims="[0]"> | |||
| <auxCoord id="cb784457" points="[ 1, 100, 3]" shape="(3,)" standard_name="time" units="Unit('hours since 1970-01-01 00:00:00', calendar='standard')" value_type="int32" var_name="time3"/> | |||
| <auxCoord id="cb784457" points="[ 1, --, 3]" shape="(3,)" standard_name="time" units="Unit('hours since 1970-01-01 00:00:00', calendar='standard')" value_type="int32" var_name="time3"/> | |||
There was a problem hiding this comment.
This may look like I'm just changing the test to suit me! But I believe my changes have actually improved how Iris works. Our previous graceful fallback on to AuxCoord involved using the DimCoord arguments for AuxCoord creation - so no masking - whereas we are now fully retrying the whole process if DimCoord fails.
There was a problem hiding this comment.
I think this is good, but see comment above.
| problem.stack_trace.__notes__ = stack_notes | ||
| problem.handled = True | ||
|
|
||
| _ = _add_or_capture( |
There was a problem hiding this comment.
This changes the behaviour slightly (as shown by your test) as the potentially masked array of point data is no longer "Gracefully filled" (line 1294ish). I think it is a good thing that masked data is retained in the AuxCoord, but maybe worth highlighting in the release notes as it might break some downstream tests?
| @@ -1554,25 +1552,15 @@ def build_and_add_auxiliary_coordinate( | |||
|
|
|||
|
|
|||
| ################################################################################ | |||
| def build_cell_measures(engine, cf_cm_var): | |||
| """Create a CellMeasure instance and add it to the cube.""" | |||
There was a problem hiding this comment.
Is it normal to ditch the docscting when making a method private?
Is it not still useful? Should it not be kept as a comment (#) perhaps?
Edit: I know they are not necessary for private methods, but we did have one, now we don't...
There was a problem hiding this comment.
It's a little unclear since this is in a private module too. I have now gone for short docstrings in the build_and_add_ functions, but no docstrings in the _build or _add 'sub-functions'.
| # Calculate the offset of each common dimension. | ||
| data_dims = [cf_var.dimensions.index(dim) for dim in common_dims] | ||
| # TODO: go the whole way and remove the 2nd parameter from _get_cf_var_data? | ||
| data = _get_cf_var_data(cf_cm_var, cf_cm_var.filename) |
There was a problem hiding this comment.
# TODO: go the whole way and remove the 2nd parameter from _get_cf_var_data?
It would be more consistent with other changes you've made w.r.t. explicitly passing in filename...
Could be raised as a separate issue to keep changes to this PR minimal?
| _ = _add_or_capture( | ||
| build_func=partial(lambda: coord_system), | ||
| add_method=partial(setattr, coord, "coord_system"), | ||
| # cf_var is usually the variable for the thing we are building. | ||
| # In this case coord_system was built earlier; cf_coord_var is here | ||
| # only to provide the filename. | ||
| cf_var=cf_coord_var, | ||
| destination=LoadProblems.Problem.Destination( | ||
| iris_class=iris.coords.DimCoord, | ||
| identifier=coord.var_name, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Is it worth commenting here that this is the "add" part of what is usually a single call to _add_or_capture and is related to the _add_or_capture call in actions.py:action_provides_grid_mapping (assuming I've understood this correctly).
| climatological=climatological, | ||
| ) | ||
|
|
||
| assert coord.var_name is not None | ||
| _ = _add_or_capture( |
There was a problem hiding this comment.
And similarly here (see above comment)
| @@ -373,8 +373,12 @@ def test_missing_latlon_radius(self): | |||
| # One of very few cases where activation may encounter an error. | |||
| # N.B. doesn't really test rules-activation, but maybe worth doing. | |||
| # (no rules trigger) | |||
| with self.assertRaisesRegex(ValueError, "No ellipsoid"): | |||
| self.run_testcase(mapping_missingradius=True) | |||
| # with self.assertRaisesRegex(ValueError, "No ellipsoid"): | |||
There was a problem hiding this comment.
Delete commented out line?
There was a problem hiding this comment.
Hey @trexfeathers
I've reviewed up to commit 8887c69 (Cell methods) and other than a few minor comments I think this is looking ace so far. 👍
trexfeathers
added
the
benchmark_this
![scitools-ci[bot]](https://avatars.githubusercontent.com/in/212324?s=60&v=4){kind=small}
🚀 Pull Request
Description
Closes #6319
Objects
LoadProblems.Problem.handledflagLoadProblems.Problem.destinationattribute, for maximum contextRefactor existing changes to capture objects being added to non-Cubeobjects tooConsult Iris pull request check list
Add any of the below labels to trigger actions on this PR: