Retrieve imagetype from casa images#532
Conversation
…nfo")` instead of `image.info()["imageinfo"]`.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| raise TypeError(f"Type {type(obj)} not serializable") | ||
|
|
||
|
|
||
| def clean_path_logic(text: str) -> str: |
There was a problem hiding this comment.
This is mainly a workaround to address the absolute path presentation in the retrieved subtable keyword value. I only became aware of this behavior until @tnakazato recently ran into a path issue, when manually registering a subtable (see this PR)
tests/unit/image/test_image.py
Outdated
| # the imagetype. | ||
| c2["imageinfo"]["imagetype"] = "Intensity" | ||
| self.dict_equality(c2, c1, "got", "expected") | ||
| with open_table_ro(self.outname()) as tb1: |
There was a problem hiding this comment.
Since im.info()["imageinfo"]["imagetype"] might not be reliable for metadata cross-checking, see #530 (comment), I now add another block to check individual keyword values from the image main table.
| coord_dict = copy.deepcopy(meta_dict["coordinates"]) | ||
| attrs = {} | ||
| attrs[_image_type] = image.info()["imageinfo"]["imagetype"] | ||
|
|
There was a problem hiding this comment.
With 8505723 now added, this change might be unnecessary to get the test working again.
I have to admit that I haven't fully understood how the 'imagetype' value persists during a round-trip conversion. @dmehring may need to look at the logic here, or perhaps we need to revert. I mainly wanted to emphasize that 'imagetype' retrieved from casacore image.info() could potentially introduce some confusion.
| else: | ||
| image_metadata = {"imagetype": "Intensity", "objectname": ""} | ||
|
|
||
| image_metadata["imagetype"] = _validate_image_type( |
There was a problem hiding this comment.
Hi @FedeMPouzols, this might be a more precise "reverse-engineering" of the python-casacore image.imageinfo underlying logic vs 78389d.
With this, imagetype retrieval for both the input and output image would end up as "Intensity" (though that may not be what we actually desired). At least this makes the casatools wrapper API behave identically to the python-casacore one.
There was a problem hiding this comment.
Aha, yes, in that commit I just tried to bring things in sync again after the recent test failures. But I see there were indeed more complications behind this and test_metadata had been passing by "luck".
I guess what remains to be figured out is whether to use imageinfo() or only directly the keywords. Perhaps long-term we should try to get support (standardize) for sky in python-casacore/casacore? Otherwise this confusion point will be there hidden and waiting to bite users.
Definitely from the code there doesn't seem to be any chance for "sky" to work via imageinfo(): https://github.com/casacore/casacore/blob/master/images/Images/ImageInfo.cc#L325 / https://github.com/casacore/casacore/blob/master/images/Images/ImageInfo.cc#L244-L254 / https://github.com/casacore/casacore/blob/master/images/Images/ImageInfo.cc#L75-L77 / https://github.com/casacore/casacore/blob/master/images/Images/ImageInfo.cc#L195-L242 /
…xds_from_casacore.py` and `tests/unit/image/test_image.py`.
| else: | ||
| attrs[_image_type] = image_type.lower() | ||
|
|
||
| attrs[_image_type] = image.info()["imageinfo"]["imagetype"] |
There was a problem hiding this comment.
Revert this to the original line since it's not essential to fix the original testing-related issue.
tests/unit/image/test_image.py
Outdated
| # from the image table, or a non-standard value of "imagetype" was | ||
| # written into that keyword, image.info() will return "Intensity" as | ||
| # the imagetype. | ||
| c2["imageinfo"]["imagetype"] = "Intensity" |
There was a problem hiding this comment.
Forcing "Intensity" is not necessary now, as the casatools wrapper will behave identically to python-casacore API. Revert back.
There was a problem hiding this comment.
Pull request overview
This PR enhances the CASA image handling in xradio by adding validation for imagetype values to match python-casacore's behavior. When reading CASA images, any non-standard imagetype values (e.g., 'sky') are now normalized to valid casacore ImageInfo enum values, with invalid types defaulting to 'Intensity'.
Changes:
- Added validation logic for
imagetypevalues when reading CASA images to match python-casacore behavior - Enhanced test infrastructure to handle path variations in metadata comparisons
- Extended metadata round-trip testing to include table keywords validation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/xradio/_utils/_casacore/casacore_from_casatools.py | Added _VALID_IMAGE_TYPES constant and _validate_image_type() function; updated imageinfo() method to validate imagetype values against casacore enum |
| tests/unit/image/test_image.py | Added clean_path_logic() helper for path normalization; enhanced dict_equality() to handle path differences; extended test_metadata() to validate table keywords |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ----- | ||
| Validation is case-insensitive. The returned string uses the | ||
| canonical capitalization from the casacore ImageInfo enum. | ||
| """ |
There was a problem hiding this comment.
The function calls value.lower() without checking if value is a string. If the imagetype keyword in a CASA image table contains a non-string value (e.g., None or an integer), this will raise an AttributeError. Consider adding type checking: if not isinstance(value, str): return "Intensity" at the beginning of the function to handle edge cases more gracefully.
| """ | |
| """ | |
| if not isinstance(value, str): | |
| return "Intensity" |
3035319 to
ecef7cc
Compare
…r keyword-based image metadata comparisons
ecef7cc to
1750c81
Compare
No description provided.