Skip to content

Fix VII interpolator producing Fortran-ordered arrays - #134

Open
djhoese wants to merge 3 commits into
pytroll:mainfrom
djhoese:bugfix-vii-c-contiguous
Open

djhoese wants to merge 3 commits into
pytroll:mainfrom
djhoese:bugfix-vii-c-contiguous

Conversation

@djhoese

@djhoese djhoese commented Sep 30, 2026

Copy link
Copy Markdown
Member

I had a case where a user of my code was using an old version of pyresample and didn't have the C-contiguous fixes that live in pyresample now (maybe just the main branch). This PR adds a C-contiguous reorder to the VII interpolator so that it doesn't have to happen else where in Satpy processing. This has the downside of data being copied an extra time when it would have been copied during a later operation anyway. It has the upside of not needing a copy later when it is needed.

Even if we decide (@ameraner, @sjoro, @pepephillips) to not merge the C-contiguous part of this I would like to commit the test changes because they are pretty helpful.

  • Closes #xxxx
  • Tests added
  • Tests passed
  • Passes git diff origin/main **/*py | flake8 --diff
  • Fully documented

@djhoese
djhoese requested a review from mraspaud September 30, 2026 15:37
@djhoese djhoese self-assigned this Sep 30, 2026
raise ValueError("The dimensions of the arrays are not consistent")

# Interpolate using the xarray interp function twice: first across, then along the scan
# Interpolate using the xarray interp function twice: first along, then across the scan

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@sjoro @pepephillips @ameraner I want to confirm this change with you. The first interp call below uses the "dim_alt" dimension and is along track, the second call uses "dim_act" so that is across track. Right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

correct yes, thanks for spotting

@codecov

codecov Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.15%. Comparing base (0a01996) to head (5c2346a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
+ Coverage   89.74%   90.15%   +0.41%     
==========================================
  Files          20       20              
  Lines        1541     1565      +24     
==========================================
+ Hits         1383     1411      +28     
+ Misses        158      154       -4     
Flag Coverage Δ
unittests 90.15% <100.00%> (+0.41%) ⬆️

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.

@mraspaud

Copy link
Copy Markdown
Member

can you explain where the problem originates from? is the data we interpolate in F-ordering to start from?

@ameraner

Copy link
Copy Markdown
Member

For the record, the mentioned fixes on pyresample side are pytroll/pyresample#723 fixed by pytroll/pyresample#725

@djhoese

djhoese commented Oct 1, 2026

Copy link
Copy Markdown
Member Author

can you explain where the problem originates from? is the data we interpolate in F-ordering to start from?

@mraspaud It comes from the scipy interp and the order that it is done in. At first in one of my earlier commits Claude had swapped it as it would fix the memory order but it makes the chunking be column based instead of row based and messes up the chunking relative to the non-interpolated datasets in the Satpy reader. See #135 for the huge rewrite version of this PR which fixes all of that at the cost of complexity.

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.

3 participants