Skip to content

Reinsert broadcasted mask#580

Merged
lkstrp merged 6 commits intomasterfrom
reinsert-broadcasted-mask
Feb 10, 2026
Merged

Reinsert broadcasted mask#580
lkstrp merged 6 commits intomasterfrom
reinsert-broadcasted-mask

Conversation

@FabianHofmann
Copy link
Collaborator

Sorry for the back and forth.

This reverts the FutureWarning from #579 and re-enables np.where (from #555) for applying masks, which indeed is significantly faster than xr.where.

The root issue was that DataArray masks weren't broadcast to match data.labels, unlike masks passed as pd.Series, pd.DataFrame, or np.ndarray (which go through as_dataarray broadcasting). This is now handled by a new broadcast_mask helper that calls broadcast_like to align/transpose the mask, then detects and warns about missing coordinates, falls back to NaN fill with False.

Dimension validation is now again a hard assert (mask.dims ⊆ data.dims) instead of a FutureWarning.

Checklist

  • Code changes are sufficiently documented; i.e. new functions contain docstrings and further explanations may be given in doc.
  • Unit tests for new features were added (if applicable).
  • A note for the release notes doc/release_notes.rst of the upcoming release is included.
  • I consent to the release of this PR's code under the MIT license.

@FabianHofmann FabianHofmann requested a review from lkstrp February 10, 2026 13:30
@FabianHofmann
Copy link
Collaborator Author

@FBumann could you also have a look. I can't select you as a reviewer here (would need to add you as a maintainer, you want that?)

@FBumann
Copy link
Contributor

FBumann commented Feb 10, 2026

@FBumann could you also have a look. I can't select you as a reviewer here (would need to add you as a maintainer, you want that?)

I'll review it this evening.
Thanks for your trust :). Maybe we can setup a call at some point and get to know each other, and then discuss this (briefly)?
I wanted to do that for some time with @lkstrp anyway. I'll write you an email.

@FBumann FBumann mentioned this pull request Feb 10, 2026
4 tasks
@FBumann
Copy link
Contributor

FBumann commented Feb 10, 2026

@FabianHofmann See #581 for small modifications. Even without: LGTM

* 1. Moved the dimension subset check into broadcast_mask
2. Added a brief docstring to broadcast_mask

* Add tests for superset dims
@lkstrp lkstrp merged commit 75c442c into master Feb 10, 2026
21 checks passed
@lkstrp lkstrp deleted the reinsert-broadcasted-mask branch February 10, 2026 15:36
@lkstrp
Copy link
Member

lkstrp commented Feb 10, 2026

Thanks for your trust :). Maybe we can setup a call at some point and get to know each other, and then discuss this (briefly)?

@FBumann We are happy to have a call to meet and align! Just send me an email again with some dates this or next week or whenever it fits you. @FabianHofmann will join as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants