Converting 3d pyresampling ravel/rearrange logic to handle nd arrays. #1457
Conversation
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
|
💖 Thanks for opening this pull request! Please check out our contributing guidelines. 💖 |
Contributor
Reviewer's GuideRefactors pyresample geocoding to support arbitrary ND input arrays by introducing reusable spatial-dimension utilities, wiring them into run_resample, and adding focused unit tests for the new helpers and shape round‑trips. Sequence diagram for updated pyresample run_resample ND array handlingsequenceDiagram
actor Caller
participant Resample
participant utils0
participant PyResample as run_pyresample
Caller->>Resample: run_resample(src_data, box_ind, print_msg)
alt software is pyresample
Resample->>utils0: move_spatial_dimension(src_data, to_front=True)
utils0-->>Resample: src_data_spatial_first
Resample->>Resample: non_spatial_shape = src_data_spatial_first.shape[2:]
Resample->>utils0: flatten_for_resample(src_data_spatial_first)
utils0-->>Resample: src_data_flat
Resample->>PyResample: run_pyresample(src_data_flat, **kwargs)
PyResample-->>Resample: dest_data_flat
Resample->>Resample: rows, cols = dest_data_flat.shape[:2]
Resample->>utils0: restore_from_resample(dest_data_flat, rows, cols, non_spatial_shape)
utils0-->>Resample: dest_data_spatial_first
Resample->>utils0: move_spatial_dimension(dest_data_spatial_first, to_front=False)
utils0-->>Resample: dest_data_original_layout
Resample-->>Caller: dest_data_original_layout
else software is scipy
alt src_data ndim > 3
Resample-->>Caller: raise NotImplementedError
else src_data ndim <= 3
Resample->>Resample: run_regular_grid_interpolator(...)
Resample-->>Caller: dest_data
end
end
Class diagram for updated resample utilities and run_resample workflowclassDiagram
class Resample {
+string software
+string interp_method
+run_resample(src_data, box_ind, print_msg)
+run_pyresample(src_data, **kwargs)
+run_regular_grid_interpolator(src_data, interp_method, fill_value, **kwargs)
}
class utils0 {
+move_spatial_dimension(data, to_front)
+flatten_for_resample(data)
+restore_from_resample(data, rows, cols, non_spatial_shape)
}
Resample ..> utils0 : uses
class move_spatial_dimension {
+np.ndarray move_spatial_dimension(data, to_front)
-np.ndarray data
-bool to_front
}
class flatten_for_resample {
+np.ndarray flatten_for_resample(data)
-np.ndarray data
}
class restore_from_resample {
+np.ndarray restore_from_resample(data, rows, cols, non_spatial_shape)
-np.ndarray data
-int rows
-int cols
-tuple non_spatial_shape
}
utils0 <.. move_spatial_dimension : defines
utils0 <.. flatten_for_resample : defines
utils0 <.. restore_from_resample : defines
Resample ..> move_spatial_dimension : move spatial dims front/back
Resample ..> flatten_for_resample : flatten nonspatial dims
Resample ..> restore_from_resample : restore nonspatial dims
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/mintpy/utils/utils0.py:1102-1076` </location>
<code_context>
+ data = data.reshape(rows, cols, -1)
+ return data
+
+def restore_from_resample(data, rows, cols, non_spatial_shape):
+ """
+ Restore non-spatial dimensions after pyresample.
+ This assumes spatial dimensions are the first two dimensions.
+
+ Parameters: data - np.ndarray, input data with spatial dimensions at the front
+ rows - int, number of rows in spatial dimensions
+ cols - int, number of columns in spatial dimensions
+ non_spatial_shape - tuple, original shape of non-spatial dimensions
+ Returns: data - np.ndarray, reshaped data with original non-spatial dimensions
+ """
+
+ if len(non_spatial_shape) > 0:
+ data = data.reshape(rows, cols, *non_spatial_shape)
+ else:
+ data = data.reshape(rows, cols)
+ return data
#################################### User Interaction #####################################
</code_context>
<issue_to_address>
**suggestion:** Consider simplifying `restore_from_resample` by deriving rows/cols from `data` instead of passing them in
Since callers always compute `rows, cols` from `dest_data.shape[:2]` and `restore_from_resample` only uses them for reshaping, it can instead read `rows, cols = data.shape[:2]` internally and just take `non_spatial_shape`. This reduces the API surface and removes the risk of `rows/cols` drifting from the actual input shape.
Suggested implementation:
```python
def restore_from_resample(data, non_spatial_shape):
"""
Restore non-spatial dimensions after pyresample.
This assumes spatial dimensions are the first two dimensions.
Parameters
----------
data : np.ndarray
Input data with spatial dimensions at the front.
non_spatial_shape : tuple
Original shape of non-spatial dimensions.
Returns
-------
np.ndarray
Reshaped data with original non-spatial dimensions restored.
"""
rows, cols = data.shape[:2]
if non_spatial_shape:
data = data.reshape(rows, cols, *non_spatial_shape)
else:
data = data.reshape(rows, cols)
return data
```
All call sites of `restore_from_resample` must be updated to match the new signature. Wherever it is currently called like:
`restore_from_resample(data, rows, cols, non_spatial_shape)`
it should be changed to:
`restore_from_resample(data, non_spatial_shape)`
and any now-unused local variables `rows` and `cols` that were only computed for this call can be removed.
</issue_to_address>
### Comment 2
<location> `tests/test_geocoding.py:7-8` </location>
<code_context>
+# from mintpy.objects.resample import resample
+from mintpy.utils.utils0 import move_spatial_dimension, flatten_for_resample, restore_from_resample
+
+def test_geocode_3d():
+ pass
+
+def test_geocode_4d():
</code_context>
<issue_to_address>
**issue (testing):** Replace placeholder geocode 3D test with an integration test that proves the >2D pyresample path works
Right now this test is just a stub, so it doesn’t actually exercise the 3D geocode/pyresample path described in the PR. Please turn it into an integration-style test that creates a minimal `resample` instance (or uses an existing fixture) configured for `pyresample`, passes in a small 3D array (e.g., `(time, rows, cols)`), runs `run_resample`, and asserts the call succeeds and the output shape is correct for a 3D input. If feasible, also check a simple value-preservation/transform case (e.g., identity LUT) to show the new utility logic is wired correctly and the regression from #1299 is covered.
</issue_to_address>
### Comment 3
<location> `tests/test_geocoding.py:10-8` </location>
<code_context>
+def test_geocode_3d():
+ pass
+
+def test_geocode_4d():
+ pass
+ # res_obj = resample(lut_file=lookupFile,
+ # src_file=file,
</code_context>
<issue_to_address>
**issue (testing):** Add a 4D geocode integration test to confirm the new ND handling fixes #1299
Given this PR is meant to fix failures for arrays with >3 dimensions, this is the key scenario to cover end-to-end.
Please implement this test to:
- Build a small 4D input array (e.g. `(time, band, rows, cols)`).
- Run it through `run_resample` using `pyresample`.
- Assert that:
- No exception is raised.
- The output shape/layout matches expectations.
- Non-spatial axes (e.g. time/band) preserve their ordering and are not swapped or collapsed.
Without this, we don’t actually verify that the ND ravel/rearrange logic fixes the original 4D failure (#1299).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ead of function args. Remove end->end tests that aren't really possible with the path inputs of mintpy
Contributor
|
If you run |
Author
|
Thanks Scott! That is very helpful in fixing those files. I guess I should have read the readme closer... |
Author
|
I tested this as a workflow using Happy to provide those files for testing if we need to do more testing than the workflow tests run already in the CI/CD. |
ZachHoppinen
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 PR adds in 3 utility functions, rearranges the py-resample ravel/rearrange logic to handle nd array, and adds in tests of the utility functions used on 2d, 3d, and 4d arrays.
This PR address the fact that currently calling geocode on any array with more than 3d dimension fails. #1299
Reminders
Summary by Sourcery
Extend pyresample-based geocoding to support arbitrarily high-dimensional input arrays by generalizing spatial dimension handling and reshaping logic.
New Features:
Bug Fixes:
Enhancements:
Tests: