536 rewrite tests/unit/image/test_imagepy to use truth images rather than hardcoded values by dmehring · Pull Request #546 · casangi/xradio

dmehring · 2026-03-06T03:00:39Z

No description provided.

codecov · 2026-03-06T03:00:50Z

Codecov Report

❌ Patch coverage is 88.09524% with 20 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/xradio/testing/assertions.py	87.73%	20 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copilot

Pull request overview

This PR rewrites the image tests in tests/unit/image/test_image.py to compare against truth datasets (stored as downloadable zarr files) rather than hardcoded expected values. It introduces a new xradio.testing module with assert_xarray_datasets_equal — a comprehensive assertion helper for comparing xarray Datasets. The PR also fixes minor consistency issues in the FITS and casacore frequency attribute schemas.

Changes:

Added src/xradio/testing/assertions.py with assert_xarray_datasets_equal and supporting comparison functions, plus comprehensive unit tests in tests/unit/testing/test_assertions.py.
Refactored tests/unit/image/test_image.py to replace hardcoded expected values and custom dict_equality/ImageBase comparison methods with the new assert_xarray_datasets_equal and _compare_attrs_dict, using downloaded truth zarr datasets.
Fixed frequency coordinate metadata inconsistencies: changed FITS type from "frequency" to "spectral_coord" and removed redundant units/frame keys from casacore frequency attrs; also fixed a bug where test_im.getdata() was compared to itself instead of expec_im.getdata().

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
`src/xradio/testing/__init__.py`	New module init exporting `assert_xarray_datasets_equal`
`src/xradio/testing/assertions.py`	New assertion helpers for xarray Dataset comparison
`tests/unit/testing/test_assertions.py`	Comprehensive unit tests for the new assertion helpers
`tests/unit/image/test_image.py`	Major rewrite replacing hardcoded values with truth dataset comparisons, lazy loading, and utility refactoring
`src/xradio/image/_util/_fits/xds_from_fits.py`	Fix frequency `type` to `"spectral_coord"` and remove `rest_frequencies`
`src/xradio/image/_util/_casacore/xds_from_casacore.py`	Remove redundant frequency attrs, dead code cleanup

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-06T03:08:18Z

src/xradio/testing/assertions.py

+            )
+        return
+
+    if _is_numeric_scalar(test) and _is_numeric_scalar(true):


math.isclose does not handle NaN values — math.isclose(NaN, NaN) returns False. This is inconsistent with _compare_arrays which uses equal_nan=True (line 214). If both test and true are NaN scalars, the assertion will incorrectly fail. Consider adding a NaN check before math.isclose, e.g., if both values are NaN, return without error.

Suggested change

if _is_numeric_scalar(test) and _is_numeric_scalar(true):

if _is_numeric_scalar(test) and _is_numeric_scalar(true):

# Handle NaN explicitly: treat two NaNs as equal, consistent with _compare_arrays(equal_nan=True)

try:

if np.isnan(test) and np.isnan(true):

return

except (TypeError, ValueError):

# If np.isnan is not applicable, fall back to math.isclose

pass

Copilot · 2026-03-06T03:08:18Z

tests/unit/image/test_image.py

+    # If obj is the detault, check for the existance of the file, if not exists(),
+    # download it, open it, and return the xds.
+    # obj: if not default, check that the object is None. If it is None, download
+    # the file, open it with open_image, setting obj equal to the returned xds.
+    # if obj is not None, this indicates the file has been downlaoded already and openend,
+    # so do nothing.
+    # if obj is default and wantreturn is False, just download the file if it doesn't exist, but don't open it or return anything.
+
+    if not os.path.exists(fname):
+        download(fname)
+        assert os.path.exists(fname), f"Cound not download {fname}"


Several spelling errors in these comments: "detault" → "default", "existance" → "existence", "downlaoded" → "downloaded", "openend" → "opened".

Suggested change

# If obj is the detault, check for the existance of the file, if not exists(),

# download it, open it, and return the xds.

# obj: if not default, check that the object is None. If it is None, download

# the file, open it with open_image, setting obj equal to the returned xds.

# if obj is not None, this indicates the file has been downlaoded already and openend,

# so do nothing.

# if obj is default and wantreturn is False, just download the file if it doesn't exist, but don't open it or return anything.

if not os.path.exists(fname):

download(fname)

assert os.path.exists(fname), f"Cound not download {fname}"

# If obj is the default, check for the existence of the file, if not exists(),

# download it, open it, and return the xds.

# obj: if not default, check that the object is None. If it is None, download

# the file, open it with open_image, setting obj equal to the returned xds.

# if obj is not None, this indicates the file has been downloaded already and opened,

# so do nothing.

# if obj is default and wantreturn is False, just download the file if it doesn't exist, but don't open it or return anything.

if not os.path.exists(fname):

download(fname)

assert os.path.exists(fname), f"Could not download {fname}"

Copilot · 2026-03-06T03:08:19Z

tests/unit/image/test_image.py

+
+    if not os.path.exists(fname):
+        download(fname)
+        assert os.path.exists(fname), f"Cound not download {fname}"


Typo: "Cound" should be "Could".

Suggested change

assert os.path.exists(fname), f"Cound not download {fname}"

assert os.path.exists(fname), f"Could not download {fname}"

Copilot · 2026-03-06T03:08:19Z

tests/unit/image/test_image.py

+                    _compare_attrs_dict(
+                        c1,
+                        c2,
+                        context=f"casa image metadata test, imnmae={imname}",


Typo: "imnmae" should be "imname" in the context string.

Copilot · 2026-03-06T03:08:19Z

tests/unit/image/test_image.py

+            _compare_attrs_dict(
+                t,
+                second_attrs,
+                context=f"masking test, cas 2a",


Typo: "cas 2a" likely should be "case 2a" in the context string.

Suggested change

context=f"masking test, cas 2a",

context=f"masking test, case 2a",

Copilot · 2026-03-06T03:08:20Z

tests/unit/image/test_image.py

+        self._fds.coords["velocity"].values = radio_velocity
+        self._fds_no_sky.coords["velocity"].values = radio_velocity
+        self._fds.coords["velocity"].attrs["doppler_type"] = "radio"
+        self._fds_no_sky.coords["velocity"].attrs["doppler_type"] = "radio"
+        # FITS SKY values are nan where the FLAG_SKY values are True, so set SKY to nan in the true xds for comparison
+        true_xds = deepcopy(self.true_xds())
+        true_xds_no_sky = deepcopy(self.true_no_sky_xds())
+        if "FLAG_SKY" in true_xds:
+            true_xds["SKY"].values = xr.where(
+                true_xds["FLAG_SKY"].values, np.nan, true_xds["SKY"]
            )
-            self.assertTrue(
-                (fds.beam_params_label == ["major", "minor", "pa"]).all(),
-                "Incorrect beam_params_label values",
+        if "FLAG_SKY" in true_xds_no_sky:
+            true_xds_no_sky["SKY"].values = xr.where(
+                true_xds_no_sky["FLAG_SKY"].values, np.nan, true_xds_no_sky["SKY"]
            )
-
-    def test_xds_attrs(self):
-        """Test xds level attributes"""
-        for fds in (self._fds, self._fds_no_sky):
-            self.compare_xds_attrs(fds)
-            self.compare_sky_attrs(fds.SKY, True)
+        # FITS gets a FITS specific user attr member added that isn't in the true data set
+        self._fds["SKY"].attrs["user"] = {}
+        self._fds_no_sky["SKY"].attrs["user"] = {}
+        assert_xarray_datasets_equal(self._fds, true_xds)
+        assert_xarray_datasets_equal(self._fds_no_sky, true_xds_no_sky)


test_got_xds mutates the class-level self._fds and self._fds_no_sky objects in-place (modifying velocity values and attrs, and setting user to {}). While these objects aren't currently used by other tests in this class, this mutation of shared state is fragile—if any future test needs the original _fds values, it will get corrupted data. Consider using deepcopy on _fds and _fds_no_sky before modifying them, similar to how true_xds and true_xds_no_sky are already deep-copied.

Copilot · 2026-03-06T03:08:20Z

tests/unit/image/test_image.py

+from xradio._utils._casacore.tables import open_table_ro

-from toolviper.dask.client import local_client
+from xradio.testing.assertions import assert_xarray_datasets_equal, _compare_attrs_dict


The private function _compare_attrs_dict is imported directly from xradio.testing.assertions. Since it's used in multiple test methods for comparing raw dicts (not full xarray datasets), consider either making it a public API in xradio.testing.__init__ (e.g., assert_attrs_dicts_equal) or adding a public wrapper function. Importing private functions from library modules makes the tests fragile against internal refactoring.

Copilot · 2026-03-06T03:08:20Z

tests/unit/image/test_image.py

            Verify fix to issue 45
            https://github.com/casangi/xradio/issues/45
-        irint("*** r", r)
+        irint("***  r)


This docstring contains corrupted text from a debug statement: irint("*** r). It appears that the original debug code irint("*** r", r) was partially cleaned up but incorrectly. The docstring should be cleaned up to only contain the description and issue link.

Copilot · 2026-03-06T03:08:20Z

tests/unit/image/test_image.py

-                            "worldreplace2",
-                        ],
+                        kw2,
+                        context=f"casa image table keyword test, imnmae={imname}",


Same typo as above: "imnmae" should be "imname" in the context string.

Copilot · 2026-03-06T03:08:20Z

src/xradio/testing/assertions.py

+            details.append(f"extra={extra}")
+        raise AssertionError(f"{context} keys mismatch: {'; '.join(details)}")
+
+    # for key in sorted(test_keys & true_keys):


The commented-out line # for key in sorted(test_keys & true_keys): should be removed. Since the keys are verified to be identical by the check above (lines 231-239), iterating over sorted(test_keys) is equivalent to iterating over sorted(test_keys & true_keys). The leftover comment is just noise.

Suggested change

# for key in sorted(test_keys & true_keys):

dmehring · 2026-03-06T03:30:06Z

The macos test failures appear to be CI env related. Because on mac the Observatories table cannot be read, the normal telescope metadata is not written to the xds. The truth image has the telescope metadata encoded, so the xds produced by the macos tests != the truth image and thus the failure.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-06T04:45:42Z

tests/unit/image/test_image.py

+
+    def test_got_xds(self):
+        # casacore writes the fits image with doppler type Z even though the casacore image
+        # uses doppler type RADIO. So that may be a casacore bug, so we need to conver the


Spelling error: "conver" should be "convert".

Suggested change

# uses doppler type RADIO. So that may be a casacore bug, so we need to conver the

# uses doppler type RADIO. So that may be a casacore bug, so we need to convert the

Copilot · 2026-03-06T04:45:43Z

tests/unit/image/test_image.py

+    @classmethod
+    def xds_uv(cls):
+        return _download(cls.uv_image(), cls._xds_uv)


The _download helper is intended to cache the opened xr.Dataset, but xds_uv(), true_xds(), true_no_sky_xds(), and true_uv_xds() never store the returned dataset back to the class variable. For example, cls._xds_uv is always None, so _download(cls.uv_image(), cls._xds_uv) will re-open the image from disk on every call. Either assign the result back (e.g., cls._xds_uv = _download(cls.uv_image(), cls._xds_uv)) or use the same if not cls._xds_uv pattern used in xds() and xds_no_sky().

Copilot · 2026-03-06T04:45:43Z

tests/unit/image/test_image.py

+    @classmethod
+    def true_xds(cls):
+        return _download(cls._xds_from_casa_true, cls._xds_true)


Same caching issue as xds_uv(): cls._xds_true is always None since the result of _download is never assigned back. Every call to true_xds() will re-open the image from disk. The same applies to true_no_sky_xds() and true_uv_xds() below.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-06T05:14:46Z

tests/unit/image/test_image.py

+            t.close()

    def compare_image_block(self, imagename, zarr=False):
        x = [0] if zarr else [0, 1]


The variable x at line 184 is assigned but never used. It was previously used to iterate over do_sky_coords=True and do_sky_coords=False cases, but the loop was removed. This is dead code and should be removed.

Suggested change

x = [0] if zarr else [0, 1]

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-06T05:25:32Z

tests/unit/image/test_image.py

+
+    def test_got_xds(self):
+        # casacore writes the fits image with doppler type Z even though the casacore image
+        # uses doppler type RADIO. So that may be a casacore bug, so we need to conver the


Typo: "conver" should be "convert".

Suggested change

# uses doppler type RADIO. So that may be a casacore bug, so we need to conver the

# uses doppler type RADIO. So that may be a casacore bug, so we need to convert the

Copilot · 2026-03-06T05:25:32Z

tests/unit/image/test_image.py

+                    assert_attrs_dicts_equal(
+                        c1,
+                        c2,
+                        context=f"casa image metadata test, imname={imname}",
+                        rtol=1e-7,
+                        atol=1e-7,
+                    )


The argument order for assert_attrs_dicts_equal is (test_attrs, true_attrs), but here c1 (the original/truth image info) is passed as test_attrs and c2 (the output/test image info) is passed as true_attrs. This reversed order won't affect pass/fail correctness since the comparison is symmetric, but it will produce misleading error messages (e.g., labeling "missing" keys as "extra" and vice versa) if a mismatch occurs. Consider swapping the arguments to assert_attrs_dicts_equal(c2, c1, ...) for clearer diagnostics.

Copilot · 2026-03-06T05:25:32Z

src/xradio/image/_util/_fits/xds_from_fits.py

        meta["rest_frequency"] = make_quantity(helpers["restfreq"], "Hz")
-        meta["rest_frequencies"] = [meta["rest_frequency"]]
-        meta["type"] = "frequency"
+        # it appears this was purged from the schema, not sure why


The comment "it appears this was purged from the schema, not sure why" is vague and suggests uncertainty. If the removal of rest_frequencies is intentional to align with the CASA code path (which also doesn't have it), consider replacing this comment with a more definitive explanation. Additionally, note that the image factory in image_factory.py:85 still sets rest_frequencies — you may want to verify if the factory should also be updated for consistency.

Suggested change

# it appears this was purged from the schema, not sure why

# rest_frequencies was removed from the schema; we now expose only

# the scalar rest_frequency here to match the CASA code path and

# current image schema.

smcastro · 2026-03-06T08:35:07Z

tests/unit/image/test_image.py

+            os.remove(filename)
+
+
+def _download(fname: str, wantreturn=True) -> xr.Dataset | None:


For future refactoring of test_image.py, it will be best to move this _download function to the testing module so that it allows external projects such as benchviper to use it in the benchmark tests without the dependency on toolviper. There was a refactoring done for the measurement_set tests and the download of the MSs was moved to src/xradio/testing/measurement_set/io.py and the tests call it through a conftest.py.

CoPilot already caught most of the small things. This branch makes great improvements to the structure of test_image.py. Once this is merged I will review my original refactoring plan and adapt to the new changes here. Thanks.

smcastro · 2026-03-06T08:42:52Z

tests/unit/image/test_image.py

+import pytest
+import re
+import shutil
+import unittest


For information, the unittest import will be removed from these tests in the next refactoring and we will rely only on pytest for test framework.

dmehring · 2026-03-18T18:18:12Z

In an attempt to bypass reliance on casa image and fits image creation within the tests, I've requested these images be uploaded so they can be directly downloaded rather than relying on casacore to create them during the tests to avoid any casacore environmental issues that might cause tests to fail that do not directly reflect on changes to xradio code (eg if the casacore Observatories table isn't present in the test environment). casangi/toolviper#44

…e-truth-images-rather-than-hardcoded-values

…ociated test workaround.

536 fix casatools backend tests

dmehring added 16 commits February 19, 2026 10:24

add helper function to __init__

9706fbd

test helpers

7e8c35e

units, frame now in other attr dicts

4993521

no more rest_frequencies in schema,freq coord type update

53a8bb5

test helpers

79397dc

incompute rewrite of tests to use truth images

73b3174

add create_empty_sky_image tests

721f664

rewrite make_empty_aperture_image tests

93c6095

rewrite make_empty_lmuv_image tests

3d2362b

refactor, remove dead code, one last test done

5282777

refactor

104e582

refactor

398919d

refactor

43d3746

clean up

f351d5a

black

3ebd703

add downloading of truth images

df8432d

dmehring linked an issue Mar 6, 2026 that may be closed by this pull request

rewrite tests/unit/image/test_image.py to use "truth" images rather than hardcoded values #536

Closed

dmehring requested a review from Copilot March 6, 2026 03:00

Copilot started reviewing on behalf of dmehring March 6, 2026 03:01 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

respond to copilot review comments

0cd118a

dmehring requested a review from Copilot March 6, 2026 04:38

Copilot started reviewing on behalf of dmehring March 6, 2026 04:38 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

more responses to copilot comments

9aceaff

dmehring requested a review from Copilot March 6, 2026 05:08

Copilot started reviewing on behalf of dmehring March 6, 2026 05:08 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

more responses to copilot comments

72532ad

dmehring requested a review from Copilot March 6, 2026 05:17

Copilot started reviewing on behalf of dmehring March 6, 2026 05:18 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

more responses to copilot comments

8ec2c87

dmehring requested review from Jan-Willem and smcastro March 6, 2026 05:30

smcastro reviewed Mar 6, 2026

View reviewed changes

dmehring and others added 5 commits March 20, 2026 13:29

transitional change, not ready for primetime

29476f6

images that were created otf now downloaded

8b22da9

Merge branch 'main' into 536-rewrite-testsunitimagetest_imagepy-to-us…

b4c8704

…e-truth-images-rather-than-hardcoded-values

Use np.atleast_1d for worldreplace coordinates and remove the ass…

2b3a0dc

…ociated test workaround.

Merge pull request #558 from casangi/536-fix-casatools-backend-tests

f484e28

536 fix casatools backend tests

Jan-Willem merged commit 9f6bc66 into main Mar 26, 2026
23 of 25 checks passed

-    if _is_numeric_scalar(test) and _is_numeric_scalar(true):
+    if _is_numeric_scalar(test) and _is_numeric_scalar(true):
+        # Handle NaN explicitly: treat two NaNs as equal, consistent with _compare_arrays(equal_nan=True)
+        try:
+            if np.isnan(test) and np.isnan(true):
+                return
+        except (TypeError, ValueError):
+            # If np.isnan is not applicable, fall back to math.isclose
+            pass

	assert os.path.exists(fname), f"Cound not download {fname}"
	assert os.path.exists(fname), f"Could not download {fname}"

	context=f"masking test, cas 2a",
	context=f"masking test, case 2a",

	# uses doppler type RADIO. So that may be a casacore bug, so we need to conver the
	# uses doppler type RADIO. So that may be a casacore bug, so we need to convert the

-        # it appears this was purged from the schema, not sure why
+        # rest_frequencies was removed from the schema; we now expose only
+        # the scalar rest_frequency here to match the CASA code path and
+        # current image schema.

		os.remove(filename)


		def _download(fname: str, wantreturn=True) -> xr.Dataset \| None:

Conversation

dmehring commented Mar 6, 2026

Uh oh!

codecov bot commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

dmehring commented Mar 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

smcastro Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Mar 6, 2026 •

edited

Loading

smcastro Mar 6, 2026 •

edited

Loading

dmehring commented Mar 18, 2026 •

edited

Loading