-
Notifications
You must be signed in to change notification settings - Fork 280
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
Partial collapse of multi-dim string coords: take 2 #5955
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5955 +/- ##
=======================================
Coverage 89.78% 89.78%
=======================================
Files 93 93
Lines 23007 23011 +4
Branches 5017 5021 +4
=======================================
+ Hits 20657 20661 +4
Misses 1620 1620
Partials 730 730 ☔ View full report in Codecov by Sentry. |
bounds = [] | ||
for index in np.ndindex(shape): | ||
index_slice = (slice(None),) + tuple(index) | ||
bounds.append(serialize(self.bounds[index_slice])) |
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 I've understood the old slicing method, it and the new method are consistent for the basic case of 2D bounds (n_points, n_bounds). For higher dimensions, the old method doesn't look right to me: if we had a 2D (n, m) coordinate with bounds (n, m, n_bounds), it looks like we would get m * n_bounds elements in the list. But I think we would have wanted just n_bounds elements.
I didn't even know bounded string coords were a thing!
# Create the new collapsed coordinate. | ||
coord = self.copy(points=np.array(points, dtype=dtype), bounds=bounds) | ||
coord = self.copy(points=np.array(points), bounds=bounds) |
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 don't know why it was necessary to set the dtype
, but the existing tests pass fine without this now. The old code is 11 years old, so I guess numpy has moved on and is likely doing more for us automatically.
For reference, here is what we currently use for string coordinates in iris/lib/iris/analysis/__init__.py Lines 2437 to 2460 in 95b7ffe
|
🚀 Pull Request
Description
This is the same bugfix as #4294. In there, I also tried to add type hints following my interpretation of the dev guide. This turned out to be complicated and I suspect is the main reason the PR has not made progress.
I think it would be good to fix the bug regardless of typing questions, so here is a version without the type hints which I'm hoping will be an easier review.
Fixes #3653.
If you have a 2D string coordinate on a cube, and collapse one of the dimensions spanned by that coordinate, the coordinate's points (and bounds, if any) are now serialised along the collapsed dimension. This seems to have been missed when partial collapse was implemented for the numeric coordinates.
Consult Iris pull request check list
Add any of the below labels to trigger actions on this PR: