536 fix casatools backend tests#558
Conversation
…ociated test workaround.
r-xue
changed the base branch from
main
to
536-rewrite-testsunitimagetest_imagepy-to-use-truth-images-rather-than-hardcoded-values
There was a problem hiding this comment.
Pull request overview
Fixes CASA/casatools round-trip metadata consistency by ensuring worldreplace* coordinate values are written with the expected array shape, allowing backend tests to compare metadata without special-casing.
Changes:
- Removed a test workaround that manually patched
coordinates["worldreplace2"]during metadata comparisons. - Updated CASA coordinate serialization to write
worldreplace1/worldreplace2as at-least-1D numpy arrays (avoids 0-d arrays from scalarcrvalvalues).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/unit/image/test_image.py | Removes the worldreplace2 metadata patch now that serialization produces comparable values. |
| src/xradio/image/_util/_casacore/xds_to_casacore.py | Writes worldreplace1/worldreplace2 via np.atleast_1d(...) to avoid 0-d array edge cases with casatools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| coord["worldreplace1"] = np.array(coord["stokes1"]["crval"]) | ||
| coord["worldreplace2"] = np.array(coord["spectral2"]["wcs"]["crval"]) | ||
| coord["worldreplace1"] = np.atleast_1d(coord["stokes1"]["crval"]) | ||
| coord["worldreplace2"] = np.atleast_1d(coord["spectral2"]["wcs"]["crval"]) |
There was a problem hiding this comment.
The coord["spectral2"]["wcs"]["crval"] value is stored as a Python float. Before the changes, calling np.array(coord["spectral2"]["wcs"]["crval"]) actually created a 0-dimensional numpy array (e.g., array(1415000000.0)).
casatools doesn't seem to like serializing 0-D arrays into the table keyword structure. By failing silently on serialization, it then later substituted the value with an empty 1-D array (array([], dtype=float64)) when fetching, therefore leading to the mismatch. python-casacore, on the other hand, might be more tolerant of this... even though I believe this should be a 1D array in the first place.
I have wrapped both coord["stokes1"]["crval"] and coord["spectral2"]["wcs"]["crval"] here with np.atleast_1d, even though it's likely unnecessary for the former.
Since we have figured out the root cause now, the earlier worldreplace2 workaround in test_image.py also got cleaned up.
|
The integration test failure seems to be a separate issue? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # some quantities are expected to have different untis and values | ||
| self._normalize_coords_for_compare(c2["coordinates"], f) | ||
| # the actual velocity values aren't stored but rather computed | ||
| # by casacore on the fly, so we cannot easily compare them, | ||
| # and really comes down to comparing the values of c used in |
There was a problem hiding this comment.
Typo in comment: "untis" should be "units".
| @@ -214,8 +214,9 @@ def _coord_dict_from_xds(xds: xr.Dataset) -> dict: | |||
| coord["worldmap2"] = np.array([3], dtype=np.int32) | |||
| # this probbably needs some verification | |||
There was a problem hiding this comment.
Typo in comment: "probbably" should be "probably".
| # this probbably needs some verification | |
| # this probably needs some verification |
|
Thanks @r-xue, looks good to me. |
dmehring
left a comment
dmehring
left a comment
There was a problem hiding this comment.
That's a tricky one. Thanks for fixing it Rui.
dmehring
merged commit f484e28
into
536-rewrite-testsunitimagetest_imagepy-to-use-truth-images-rather-than-hardcoded-values
No description provided.