Skip to content
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

Merged
merged 13 commits into from
Aug 30, 2024

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented May 15, 2024

🚀 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:

  • benchmark_this Request that this pull request be benchmarked to check if it introduces performance shifts

Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.78%. Comparing base (5cc6737) to head (bd30394).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5955   +/-   ##
=======================================
  Coverage   89.78%   89.78%           
=======================================
  Files          88       88           
  Lines       23056    23060    +4     
  Branches     5027     5031    +4     
=======================================
+ Hits        20700    20704    +4     
  Misses       1624     1624           
  Partials      732      732           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/iris/coords.py Show resolved Hide resolved
lib/iris/coords.py Show resolved Hide resolved
@rcomer
Copy link
Member Author

rcomer commented May 15, 2024

For reference, here is what we currently use for string coordinates in aggregated_by, which handles 2D coords but not bounded string coords.

if coord.points.dtype.kind in "SU":
if coord.bounds is None:
new_points_list = []
new_bounds = None
# np.apply_along_axis does not work with str.join, so we
# need to loop through the array directly. First move axis
# of interest to trailing dim and flatten the others.
work_arr = np.moveaxis(coord.points, dim, -1)
shape = work_arr.shape
work_shape = (-1, shape[-1])
new_shape: tuple[int, ...] = (len(self),)
if coord.ndim > 1:
new_shape += shape[:-1]
work_arr = work_arr.reshape(work_shape)
for indices in self._groupby_indices:
for arr in work_arr:
new_points_list.append("|".join(arr.take(indices)))
# Reinstate flattened dimensions. Aggregated dim now leads.
new_points = np.array(new_points_list).reshape(new_shape)
# Move aggregated dimension back to position it started in.
new_points = np.moveaxis(new_points, 0, dim)

@stephenworsley stephenworsley self-assigned this Aug 29, 2024
lib/iris/coords.py Outdated Show resolved Hide resolved
@rcomer rcomer force-pushed the collapse-2d-string-coord-untyped branch from 0079245 to 638d491 Compare August 30, 2024 10:43
Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@stephenworsley stephenworsley merged commit 44257eb into SciTools:main Aug 30, 2024
21 checks passed
@rcomer
Copy link
Member Author

rcomer commented Aug 30, 2024

Thanks @stephenworsley! 🎉

@rcomer rcomer deleted the collapse-2d-string-coord-untyped branch August 30, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cube.collapsed fails with multi-dimensional string coordinates
2 participants