Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Fixes loading of CASA “sumwt” / VISIBILITY_NORMALIZATION images in load_image by ensuring singleton spatial axes are removed so the resulting xarray variables match the canonical (time, frequency, polarization) dimensionality.
Changes:
- Add a unit test asserting
VISIBILITY_NORMALIZATIONblock loads squeeze awayl/maxes. - Apply
_squeeze_if_neededto mask blocks as well during CASA block loading. - Change
_get_persistent_blockto return adask.array.Array(instead of wrapping in anxr.DataArray) to support the squeeze workflow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/unit/image/test_image.py |
Adds coverage for squeezing singleton spatial axes when loading visibility normalization images. |
src/xradio/image/_util/casacore.py |
Ensures mask blocks are also squeezed for VISIBILITY_NORMALIZATION in block-loading path. |
src/xradio/image/_util/_casacore/xds_from_casacore.py |
Adjusts persistent-block helper to return a dask array rather than an xarray wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| block = _get_persistent_block( | ||
| full_path, shapes, starts, dimorder, transpose_list, new_axes | ||
| ) | ||
| block = _squeeze_if_needed(block, image_type) | ||
| # data vars are all caps by convention |
There was a problem hiding this comment.
The newly added _squeeze_if_needed(...) call can trigger an exception path where _squeeze_if_needed attempts to build the ValueError message via string concatenation with a Python list ("... Found " + [shape[3], shape[4]]), which will raise a TypeError and mask the intended validation error. Please format the shape values as a string (e.g., tuple/list via f-string/str(...)) so the correct ValueError is raised when l/m are not length 1 (and consider guarding against unexpected dimensionality before indexing shape[3]/shape[4]).
| block = _get_persistent_block( | ||
| full_path, shapes, starts, dimorder, transpose_list, new_axes | ||
| ) | ||
| block = _squeeze_if_needed(block, image_type) | ||
| # data vars are all caps by convention |
There was a problem hiding this comment.
This new squeeze step inside the mask loop isn’t covered by the added unit test (the synthetic sumwt image in the test has no mask, so the loop never executes). Consider extending/adding a test that creates a VISIBILITY_NORMALIZATION image with a mask (e.g., MASK_0/default mask) and asserts the returned flag/mask data variable is also squeezed to (time, frequency, polarization) with no l/m dims.
| def _get_persistent_block( | ||
| infile: str, | ||
| shapes: tuple, | ||
| starts: tuple, | ||
| dimorder: list, | ||
| transpose_list: list, | ||
| new_axes: list, | ||
| ) -> xr.DataArray: | ||
| ) -> da.Array: | ||
| block = _read_image_chunk(infile, shapes, starts) | ||
| block = np.expand_dims(block, new_axes) | ||
| block = block.transpose(transpose_list) | ||
| block = da.from_array(block, chunks=block.shape) |
There was a problem hiding this comment.
After changing _get_persistent_block to return a dask.array.Array, the dimorder parameter is no longer used in the function body. To reduce confusion and avoid unused-parameter warnings, consider either removing dimorder from the signature (and updating call sites) or renaming it to _dimorder to make the intentional non-use explicit.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.