Conversation
There was a problem hiding this comment.
Pull request overview
Fixes broken CLI imports and tightens the orientation contract for GEE tile fetches. Previously GEEProvider.fetch_array_chw returned south-up data and relied on its caller (_fetch_provider_array_chw_with_bbox_fallback) to apply _flip_sample_tile_y, which was a leaky abstraction. The flip now happens inside fetch_array_chw, the wrapper no longer flips, and a regression test pins down the (already north-up) pass-through behaviour of _fetch_all_bands_impl.
Changes:
- Move
_flip_sample_tile_yinvocation intoGEEProvider.fetch_array_chw; remove the duplicate flip from_fetch_provider_array_chw_with_bbox_fallback. - Document the orientation contract in
fetch_array_chw,_sample_image_bands_raw_chw, and_flip_sample_tile_y; warn against adding.clip()to the multiframe path. - Update test fakes to produce north-up data, rename three tests accordingly, and add
test_fetch_all_bands_impl_passes_through_gee_row_order.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/rs_embed/providers/gee.py | fetch_array_chw now flips internally and documents the north-up contract; comment added at _fetch_all_bands_impl reproject call. |
| src/rs_embed/providers/gee_utils.py | Caller stops flipping; new docstrings on _sample_image_bands_raw_chw and _flip_sample_tile_y. |
| tests/test_api_helpers.py | Fake providers updated to emit north-up rows; three tests renamed to drop "south-up" wording. |
| tests/test_gee_provider.py | New regression test asserting _fetch_all_bands_impl preserves GEE's row order. |
| CHANGELOG.md | Documents CLI import fix and the tightened fetch_array_chw contract. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+407
to
+418
| """Flip the penultimate (height) axis of a CHW or TCHW array from south-up to north-up. | ||
|
|
||
| Called by ``_fetch_provider_array_chw_with_bbox_fallback`` (the single-frame | ||
| leaf-fetch layer) to normalise the raw south-up output of | ||
| ``GEEProvider.fetch_array_chw``. ``fetch_array_chw`` uses | ||
| ``ee.Projection.atScale() + .clip(region)`` before ``sampleRectangle``, | ||
| which causes GEE to return rows in south-up order. | ||
|
|
||
| ``_sample_image_bands_raw_chw`` uses a different call pattern | ||
| (``reproject(crs=..., scale=...)`` **without** clip) that already returns | ||
| north-up rows from GEE; applying this flip there would invert those tiles. | ||
| """ |
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.
Summary
Refactor — GEE tile orientation
Previously, GEEProvider.fetch_array_chw returned south-up raw data and the flip was applied by its caller (_fetch_provider_array_chw_with_bbox_fallback). This was a leaky abstraction: any subclass overriding fetch_array_chw had to know to return south-up or risk being flipped twice.
fetch_array_chw now applies _flip_sample_tile_y internally and contracts to return north-up CHW. The caller no longer flips.
The _sample_image_bands_raw_chw docstring documents why no flip is needed there: reproject(crs=..., scale=...) without .clip() already produces north-up rows from GEE, and adding .clip() would silently invert the row order and break the multiframe (TCHW) path.
Testing
Fake provider fetch_array_chw implementations in test_api_helpers.py updated to return north-up data directly, matching the new contract; 3 test functions renamed to remove references to "flipping south-up tiles"
test_gee_provider.py: added test_fetch_all_bands_impl_passes_through_gee_row_order to lock in the pass-through behaviour of _fetch_all_bands_impl and document that its north-up guarantee comes from GEE's empirical API behaviour, not from Python-level normalization
CHANGELOG.mdfor user-facing API, model, semantic, or installation changes.Notes
Anything reviewers should know about scope, tradeoffs, or follow-up work.