585 replace xradio casacore backends#586
Open
Jan-Willem wants to merge 3 commits into
Open
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Collaborator
|
This looks pretty impressive! Was this one single iteration with Opus? About the largest changes in this PR:
It would be interesting to ask copilot for a review! |
| > with a CASA install and works the same on Linux and macOS. There is nothing to | ||
| > configure: installing `xradio[casacore]` (which pulls in `casacoretables`) is | ||
| > all that is required. This guide is kept only for historical reference. | ||
|
|
Collaborator
There was a problem hiding this comment.
I wonder if this is just removed, should we have this doc and the deprecation warning on top, or just remove it?
| """casacoretables-backed replacements for casacore's MeasurementSet helpers. | ||
|
|
||
| ``casacoretables`` deliberately ships only casacore's *table* layer, not the | ||
| MeasurementSet (``ms``) module, so the helpers python-casacore exposes for |
Collaborator
There was a problem hiding this comment.
This sounds as referring to a python-casacore MS module that doesn't seem to exist. I guess it means the MS C++ code in casacore.
| from casacore.tables.msutil import complete_ms_desc, makearrcoldesc, required_ms_desc | ||
| from casacoretables import tables | ||
| from casacoretables.tables import makedminfo, maketabdesc, makearrcoldesc | ||
| from xradio.testing.measurement_set._casacore_ms import ( |
Collaborator
There was a problem hiding this comment.
A better place for these would be a casacoretables tableutil/msutil or similar?
Member
There was a problem hiding this comment.
I agree that this functionality should be somewhere else and not in the testing module so that it can be used by code other than tests. Test testing module should contain only helper functions for tests.
| } | ||
|
|
||
|
|
||
| def _to_deg(value, unit_str): |
Collaborator
There was a problem hiding this comment.
The use of type hints looks inconsistent/incomplete throughout the files.
|
|
||
| if multibeam: | ||
| hdus.append(_beams_hdu(imageinfo["perplanebeams"])) | ||
| elif "restoringbeam" in imageinfo: |
Collaborator
There was a problem hiding this comment.
As mentioned there is a considerable drop in test coverage. Many blocks of code in this new _utils/_casacore/casa_images are never exercised apparently. This "restoringbeam" if branch is one example.
| return [coord.get_axes() for coord in self] | ||
|
|
||
|
|
||
| class coordinate(object): |
Collaborator
There was a problem hiding this comment.
These coordinate, directioncoordinates, spectralcoordinate etc. classes below are another example of code/responsibility that is transferred to xradio, and where nearly nothing seems to be exercised in the tests.
| # CASA-symbol-clash-free build of casacore's table system (replaces both | ||
| # python-casacore and casatools, on Linux and macOS alike). NOTE: PyPI wheels for | ||
| # casacoretables are not published yet, so `pip install xradio[casacore]` cannot | ||
| # resolve it from PyPI until they are; install it from source in the meantime. |
Collaborator
There was a problem hiding this comment.
This NOTE is outdated I think.
Collaborator
|
Also, if this PR will completely remove casatools and python-casacore, the github workflow "python-testing-casatools" could/should be removed (and probably also the underlying templates from the nrao repos). |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates xradio’s casacore integrations onto the casacoretables backend, removing the prior python-casacore / casatools fallback layer and adding internal replacements for the MeasurementSet helper APIs and CASA paged-image handling.
Changes:
- Replace
casacore/casatoolsfallback imports withcasacoretablesacross MSv2 and image utilities and tests. - Add
casacoretables-backed reimplementations for MS helper functions (default_ms*,*_ms_desc) and CASA paged-image I/O (including FITS export). - Update packaging/docs to include the vendored MS descriptors and document the new backend expectations.
Reviewed changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/image/test_image.py | Switch test imports to casacoretables.tables. |
| tests/stakeholder/test_measure_set_stakeholder.py | Improve assertion message on visibility/weight sum changes. |
| src/xradio/testing/measurement_set/msv2_io.py | Use casacoretables and internal MS helper replacements for synthetic MS generation. |
| src/xradio/testing/measurement_set/_ms_descriptors.json | Add vendored MS table descriptors for MS helper reimplementation. |
| src/xradio/testing/measurement_set/_casacore_ms.py | Add descriptor-driven replacements for casacore MS helper functions. |
| src/xradio/testing/measurement_set/init.py | Update warning text to refer to casacoretables. |
| src/xradio/measurement_set/_utils/_msv2/partition_queries.py | Replace backend imports with casacoretables. |
| src/xradio/measurement_set/_utils/_msv2/msv4_info_dicts.py | Replace backend imports with casacoretables. |
| src/xradio/measurement_set/_utils/_msv2/conversion.py | Replace backend imports with casacoretables. |
| src/xradio/measurement_set/_utils/_msv2/_tables/table_query.py | Replace backend imports with casacoretables. |
| src/xradio/measurement_set/_utils/_msv2/_tables/read.py | Replace backend imports and switch from getcolnp to getcol for Dask/multiprocess correctness. |
| src/xradio/measurement_set/_utils/_msv2/_tables/read_main_table.py | Replace backend imports with casacoretables. |
| src/xradio/image/_util/image_factory.py | Make image-type detection use basename to avoid path-component false positives. |
| src/xradio/image/_util/casacore.py | Replace backend imports with casacoretables. |
| src/xradio/image/_util/_casacore/xds_to_casacore.py | Replace backend imports with casacoretables. |
| src/xradio/image/_util/_casacore/xds_from_casacore.py | Replace backend imports; route image/coords through new casa_images implementation. |
| src/xradio/image/_util/_casacore/common.py | Route image API usage through xradio._utils._casacore.casa_images. |
| src/xradio/_utils/_casacore/tables.py | Replace backend imports with casacoretables. |
| src/xradio/_utils/_casacore/casacore_from_casatools.py | Remove the casatools→casacore adapter layer. |
| src/xradio/_utils/_casacore/casa_images/init.py | Add casacore.images-compatible shim package. |
| src/xradio/_utils/_casacore/casa_images/image.py | Implement CASA paged-image read/write on top of casacoretables. |
| src/xradio/_utils/_casacore/casa_images/coordinates.py | Vendor python-casacore coordinate wrappers (LGPLv3). |
| src/xradio/_utils/_casacore/casa_images/_tofits.py | Implement FITS export compatible with expectations in xradio tests. |
| src/xradio/_utils/_casacore/casa_images/_default_coords.py | Implement default coordinate record creation for newly created CASA images. |
| src/xradio/_utils/_casacore/casa_images/_coords_postprocess.py | Post-process coordinate records (native pole derivation, normalization). |
| README.md | Document casacoretables backend and updated platform support expectations. |
| pyproject.toml | Swap optional deps to casacoretables; include MS descriptor JSON in package data. |
| MANIFEST.in | Include MS descriptor JSON in source distributions. |
| docs/source/overview.rst | Update backend installation guidance to casacoretables. |
| docs/source/measurement_set/overview.rst | Mark backend replacement as done and describe casacoretables. |
| docs/source/measurement_set/guides/backends.md | Deprecate old selectable-backend guide in favor of casacoretables. |
| docs/source/development.rst | Update development/install notes to casacoretables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+411
to
+416
| assert sum == pytest.approx(expected_sum_value, rel=relative_tolerance), ( | ||
| "VISIBILITY and WEIGHT values have changed from " | ||
| + str(expected_sum_value) | ||
| + "to" | ||
| + str(sum) | ||
| ) |
Comment on lines
+71
to
+74
| def _make_table(name: str, tabdesc: dict, dminfo: dict = None): | ||
| if not dminfo: | ||
| dminfo = makedminfo(tabdesc) | ||
| return tables.table(str(name), tabdesc, nrow=0, dminfo=dminfo, ack=False) |
Comment on lines
+327
to
+330
| def getdata(self, blc=(), trc=(), inc=()) -> np.ndarray: | ||
| """Get the full image pixel array (numpy order).""" | ||
| tb = self._ro_table() | ||
| return tb.getcell(tb.colnames()[0], 0) |
Comment on lines
+349
to
+356
| with tables.table( | ||
| os.sep.join([self._path, maskname]), | ||
| readonly=True, | ||
| lockoptions={"option": "usernoread"}, | ||
| ack=False, | ||
| ) as tb: | ||
| casa_mask = tb.getcell(tb.colnames()[0], 0) | ||
| # casacore True == good -> numpy True == bad |
Comment on lines
+1
to
+12
| # coordinates.py: Python coordinate system wrapper | ||
| # | ||
| # Vendored, essentially verbatim, from python-casacore | ||
| # (casacore/images/coordinates.py, LGPL v3). This module is *pure Python*: it | ||
| # only dissects the coordinate-system record (the ``coords`` table keyword of a | ||
| # CASA image) returned by :class:`xradio._utils._casacore.casa_images.image`. | ||
| # It does not depend on the casacore C++ images library, so it works unchanged | ||
| # on top of ``casacoretables``. | ||
| # | ||
| # Original copyright (C) 2008 Associated Universities, Inc. Washington DC, USA. | ||
| # This library is free software under the GNU Lesser General Public License; | ||
| # see the casacore distribution for the full text. |
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.
https://github.com/nrao/casacoretables