Fix negligible coordinate dimension matching by sylvesterkaczmarek · Pull Request #3447 · pytroll/satpy

sylvesterkaczmarek · 2026-08-20T00:02:57Z

CompositeBase.drop_coordinates currently checks only whether a coordinate name also appears in DataArray.dims. This can preserve time(y) whenever the array separately has a time dimension, even though that coordinate does not describe the time dimension.

This changes the check to preserve a negligible coordinate only when its own dimensions are exactly its same-named dimension, for example time(time).

Tests cover:

dropping time(y) without a time dimension
dropping time(y) when a separate time dimension exists
preserving a real time(time) dimension coordinate

Testing:

Python syntax checks for the modified module and new test
focused xarray reproduction for all three coordinate cases

codecov · 2026-08-20T00:10:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.34%. Comparing base (26c746c) to head (f091316).
⚠️ Report is 86 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3447   +/-   ##
=======================================
  Coverage   96.33%   96.34%           
=======================================
  Files         467      465    -2     
  Lines       59142    59168   +26     
=======================================
+ Hits        56977    57007   +30     
+ Misses       2165     2161    -4

Flag	Coverage Δ
behaviourtests	`3.58% <5.88%> (+<0.01%)`	⬆️
unittests	`96.43% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

djhoese · 2026-08-20T02:05:16Z

This can preserve time(y) whenever the array separately has a time dimension, even though that coordinate does not describe the time dimension.

Sorry, I'm confused by this. So we have DataArray A that has dimensions (time, y, x) and one of its coordinates is "time" which is a DataArray with dimensions (y,)?

sylvesterkaczmarek · 2026-08-20T12:26:02Z

This can preserve time(y) whenever the array separately has a time dimension, even though that coordinate does not describe the time dimension.

Sorry, I'm confused by this. So we have DataArray A that has dimensions (time, y, x) and one of its coordinates is "time" which is a DataArray with dimensions (y,)?

Yes, exactly. xarray allows a DataArray with dimensions (time, y, x) to have a coordinate named time whose own dimensions are (y,). That is the second reproducer in #3353.

The current logic only checks whether the coordinate name time exists in ds.dims, so it incorrectly treats that time(y) coordinate as the coordinate variable for the time dimension. This PR instead checks the coordinate's own dimensions and only preserves the true dimension-coordinate case time(time).

djhoese · 2026-08-20T17:58:54Z

That is a strange case and it seems I said the same thing in my comment in #3353. Unfortunately @gerritholl is going on holiday for a while so we can't get his perspective on the issue as he saw it.

Two requests:

Could you move the tests into an existing test module? We typically don't make test modules for specific cases, but more for specific units/modules of code.
What do you think about making a helper function or multiple helper functions to implement this filtering. For example a _is_coord_var or something?

sylvesterkaczmarek · 2026-08-20T19:00:46Z

That is a strange case and it seems I said the same thing in my comment in #3353. Unfortunately @gerritholl is going on holiday for a while so we can't get his perspective on the issue as he saw it.

Two requests:

Could you move the tests into an existing test module? We typically don't make test modules for specific cases, but more for specific units/modules of code.

What do you think about making a helper function or multiple helper functions to implement this filtering. For example a _is_coord_var or something?

Thanks, both suggestions make sense. I’ve moved the regression cases into the existing test_core.py module alongside the other CompositeBase coordinate tests, and extracted the coordinate-variable check into _is_coord_var.

The filtering now reads explicitly in terms of whether the coordinate is the variable for its same-named dimension. CI is rerunning now.

Manny7717

Verified locally on CPU (head f091316 vs the pre-fix core.py blob from the PR diff base 1b124eb88a).

Bug is real: old logic kept any negligible coord whose NAME is a dim (coord not in ds.dims was False), so time(y) riding on an array that separately has a time dimension survived drop_coordinates even though it does not describe the time dimension. The new _is_coord_var check (ds.coords[name].dims == (name,)) is the correct criterion for "this coordinate is the variable for its own dimension".

Regression proof (executed): the 3 new tests pass at head; with the pre-fix drop_coordinates logic, test_drop_coordinates_drops_mismatched_same_named_coordinate FAILS (the genuine #3353 case — time(y) retained alongside a real time dim) while the other two pass pre-fix as expected (they lock pre-existing behavior: true dimension coord kept, non-dim time(y) without a time dim dropped). Honest, targeted coverage.

No collateral: full satpy/tests/compositor_tests/test_core.py → 36/36 at head. NEGLIGIBLE_COORDS = ["time"] substring matching is unchanged; the helper only tightens the retention rule for same-named coords on foreign dims.

Not stale: the PR diff-base core.py is byte-identical to current main's core.py except the removed GPL header (relicense #3451), so the change applies cleanly post-relicense; mergeable=MERGEABLE.

Repo CI already fully green on this head (CodeFactor, CodeScene, codecov, coveralls, RTD, pre-commit.ci, full test matrix). No human reviews yet — this is the first independent human-equivalent verification.

djhoese · 2026-09-14T14:36:31Z

@gerritholl I think you're back at work right? Any chance you have time to review this since it was your original issue?

djhoese · 2026-09-14T14:51:28Z

I'm sorry it has been a while, but I have one more request. Could you combine these 3 new tests into a single test and use pytest's parametrize? The data creation is very similar and the checks are very similar so I'm hoping it only needs to be 2-4 arguments and maybe even improved checks (checking dims and coords in the result).

sylvesterkaczmarek added 2 commits August 20, 2026 00:55

Fix coordinate dimension matching for drop_coordinates

e9c111e

Add drop_coordinates regression tests

2b48b0a

sylvesterkaczmarek requested review from djhoese and mraspaud as code owners August 20, 2026 00:02

Address review feedback on coordinate filtering

0df7cd3

sylvesterkaczmarek force-pushed the fix-3353-drop-coordinates branch from ce80651 to 0df7cd3 Compare August 20, 2026 18:29

Restore final newline in core.py

f091316

djhoese added bug component:compositors labels Aug 21, 2026

djhoese self-assigned this Aug 21, 2026

Manny7717 approved these changes Sep 5, 2026

View reviewed changes

djhoese requested a review from gerritholl September 14, 2026 14:36

djhoese reviewed Sep 14, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix negligible coordinate dimension matching - #3447

Fix negligible coordinate dimension matching#3447
sylvesterkaczmarek wants to merge 4 commits into
pytroll:mainfrom
sylvesterkaczmarek:fix-3353-drop-coordinates

sylvesterkaczmarek commented Aug 20, 2026

Uh oh!

codecov Bot commented Aug 20, 2026 •

edited

Loading

Uh oh!

djhoese commented Aug 20, 2026

Uh oh!

sylvesterkaczmarek commented Aug 20, 2026

Uh oh!

djhoese commented Aug 20, 2026

Uh oh!

sylvesterkaczmarek commented Aug 20, 2026

Uh oh!

Manny7717 left a comment

Uh oh!

djhoese commented Sep 14, 2026

Uh oh!

djhoese Sep 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

sylvesterkaczmarek commented Aug 20, 2026

Uh oh!

codecov Bot commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

djhoese commented Aug 20, 2026

Uh oh!

sylvesterkaczmarek commented Aug 20, 2026

Uh oh!

djhoese commented Aug 20, 2026

Uh oh!

sylvesterkaczmarek commented Aug 20, 2026

Uh oh!

Manny7717 left a comment

Choose a reason for hiding this comment

Uh oh!

djhoese commented Sep 14, 2026

Uh oh!

djhoese Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Aug 20, 2026 •

edited

Loading