Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
==========================================
+ Coverage 89.74% 90.54% +0.79%
==========================================
Files 20 20
Lines 1541 1671 +130
==========================================
+ Hits 1383 1513 +130
Misses 158 158
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an evolution of #134 but basically a whole rewrite for better performance and better results at the edge of scans. This is entirely Claude written with some intervention by me. Claude basically got rid of the scipy interp1d usage and wrote it manually. This concerned me, but I asked Claude to justify it (see bottom section).
CC @ameraner @sjoro @pepephillips
Here is Claude's summary:
Interpolate VII tie points per scan with dask
map_blocksThis replaces the two
xarray.DataArray.interp()passes inviiinterpolatorwith a small numpy kernel that bilinearly interpolates one scan at a time. Dask input goes throughda.map_blocks. It builds onbugfix-vii-c-contiguousand removes that branch's_as_c_contiguouscopy, which is no longer needed.Nothing changes in the public API.
tie_points_interpolationandtie_points_geo_interpolationkeep their signatures and still returnDataArrays. Those keep the tie points' dimension names, name, attrs and non-dimension coordinates.How the interpolation works
A METimage scan is 24 pixel rows with 4 tie rows. The tie rows sit at pixel positions 0, 8 and 16 of the scan, plus a trailing edge row at 24. Scans are not contiguous because of the bowtie effect, so every pixel should depend only on the tie rows of its own scan. The old code got this by putting all tie rows on one evenly spaced grid and skipping the gap between scans. The new code reshapes the tie points into scans instead, so the interpolation cannot reach outside a scan.
Problems with the old implementation that this fixes
Missing (NaN) tie points leaked into valid pixels, including across scans.
a + (b - a) * w. For a pixel exactly on a tie point,wis 0 or 1, so the other tie point should drop out. ButNaN * 0is still NaN.interp1dputs a pixel on tie point i in the interval (i−1, i) withw = 1. A NaN at i−1 therefore blanks the pixel.a + (b - a)not always rounding back to exactlyb.Real-world impact: on a CONUS granule whose first scan is missing, the old code blanked the first row of scan 1. pyresample's EWA builds each scan's footprint ellipses from the scan's first and last rows. With a NaN there, it falls back to a flat box footprint for the whole scan. In a Polar2Grid run (
vii_865, EWA, fixed grid):Chunking did not follow scans.
.interp()merges the interpolated dimension into one chunk and splits the other. The old output came back as rows of 2052 pixels (85.5 scans), whatever the input chunks were._rechunk_to_pixel_gridbecomes a no-op: 0 rechunk layers instead of 5.scanline_mapblocksdoes for MODIS.Blocks were not C-contiguous.
ll2crwithValueError: ndarray is not C-contiguous. This hit Polar2Grid users with orthorectification off.tie_points_geo_interpolationcomputed dask input while building the graph, twice, to choose between geodetic and cartesian interpolation. (a or bon a 0-dDataArrayevaluated the first reduction twice.) Satpy paid for this in the METimage file handler's__init__for every granule.map_blocksas 0-d dask arrays.Scenefor this granule goes from 6 dask computes to 4, and from 0.093 s to 0.052 s. The remaining 4 are Satpy's own reads of the calibration coefficients.Output dtype was inconsistent.
Smaller fixes:
ValueErrormessage about scan multiples was never formatted, because the format argument was passed as a second argument.Results are unchanged otherwise. The geodetic path is bit-identical to before, and the cartesian path is within 3.6e-14°. Results were already independent of chunking; that is now enforced by tests.
Performance
These numbers come from one real granule: 1368×394 tie points interpolated to 8208×3144 pixels. Test setup:
tracemalloc.tie_points_geo_interpolationTimes include building the graph, so the old version's decision compute is counted. Of the new version's tasks, 12 (75MiB) or 26 (32MiB) are the small lazy reductions.
metimage_l1b_ncfile handler lon/lat (75MiB, cartesian because the granule reaches 68°N): wall time 0.97 → 0.99 s, 1-thread time 2.01 → 1.28 s, peak memory 2512 → 1242 MiB, 118 → 22 tasks.np.wheredid. The high-latitude formula is now only applied where it's needed.Decisions still to make
Follow-ups elsewhere
metimage_nc.py:_rechunk_to_pixel_gridand its docstring (aboutxarray.interpdividing chunks evenly) can go.Testing
test_viiinterpolator.pyhas 52 tests, each run with numpy input and with dask input. They cover:🤖 Generated with Claude Code
Rewrite of interpolation kernel
Where both are valid, the values are bit-identical.
How to justify it to the original authors:
Suggested paragraph for the PR:
Why not scipy/numpy per block? Tie points and pixels are on fixed regular grids, so every pixel sits at a known fraction (k/tie_points_factor) between two tie points, and linear interpolation reduces to a constant-weight blend. _interpolate_intervals applies exactly the formula interp1d(kind="linear") uses (y0 + (y1 - y0) * w) and gives bit-identical values, with no interval search. Calling interp1d per block instead is about 1.4× slower (1.8× with the copy it would need). It returns Fortran-ordered arrays, and it keeps the zero-weight NaN leak across scans, because it assigns a pixel sitting exactly on a tie point to the interval on its left. np.interp only handles 1-D arrays.
git diff origin/main **/*py | flake8 --diff