-
Notifications
You must be signed in to change notification settings - Fork 15
564 refactor image tests #569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
smcastro
wants to merge
6
commits into
main
Choose a base branch
from
564-refactor-image-tests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
af0c694
Refactored test_image.py and moved helper functions to testing/image …
smcastro 3154032
Fixed with black.
smcastro 488b2aa
Update src/xradio/testing/image/io.py
smcastro 8ec0b7f
Copilot reviews 2 and 3. Fixed minor import things.
smcastro cd4d693
These changes address the second code review.
smcastro 03775ff
Added new unit tests to cover the new testing.image module.
smcastro File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,12 @@ | ||
| from xradio.testing.assertions import ( | ||
| from .assertions import ( | ||
| assert_attrs_dicts_equal, | ||
| assert_xarray_datasets_equal, | ||
| ) | ||
| from . import image | ||
|
|
||
| __all__ = [ | ||
| "assert_attrs_dicts_equal", | ||
| "assert_xarray_datasets_equal", | ||
| # image sub-package (imported so external projects can use it) | ||
| "image", | ||
| ] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| """ | ||
| Test utilities for xradio image functionality. | ||
|
|
||
| Usable in pytest, ASV benchmarks, and any other framework – no pytest | ||
| dependency is introduced by importing this package. | ||
|
|
||
| Examples | ||
| -------- | ||
| >>> from xradio.testing.image import ( | ||
| ... download_image, | ||
| ... download_and_open_image, | ||
| ... remove_path, | ||
| ... make_beam_fit_params, | ||
| ... create_empty_test_image, | ||
| ... create_bzero_bscale_fits, | ||
| ... scale_data_for_int16, | ||
| ... normalize_image_coords_for_compare, | ||
| ... assert_image_block_equal, | ||
| ... ) | ||
| """ | ||
|
|
||
| __all__ = [ | ||
| # IO helpers | ||
| "download_image", | ||
| "download_and_open_image", | ||
| "remove_path", | ||
| # Generators | ||
| "make_beam_fit_params", | ||
| "create_empty_test_image", | ||
| "create_bzero_bscale_fits", | ||
| "scale_data_for_int16", | ||
| # Assertions / comparators | ||
| "normalize_image_coords_for_compare", | ||
| "assert_image_block_equal", | ||
| ] | ||
|
|
||
| from xradio.testing.image.assertions import ( | ||
| assert_image_block_equal, | ||
| normalize_image_coords_for_compare, | ||
| ) | ||
| from xradio.testing.image.generators import ( | ||
| create_bzero_bscale_fits, | ||
| create_empty_test_image, | ||
| make_beam_fit_params, | ||
| scale_data_for_int16, | ||
| ) | ||
| from xradio.testing.image.io import ( | ||
| download_and_open_image, | ||
| download_image, | ||
| remove_path, | ||
| ) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| """Image-specific assertion and comparison helpers. | ||
|
|
||
| All functions raise ``AssertionError`` on failure and are framework-agnostic, | ||
| so they work equally in pytest, unittest, and ASV benchmarks. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import Dict | ||
|
|
||
| import numpy as np | ||
| import xarray as xr | ||
|
|
||
|
|
||
| def normalize_image_coords_for_compare( | ||
| coords: dict, | ||
| factor: float = 180 * 60 / np.pi, | ||
| direction_key: str = "direction0", | ||
| spectral_key: str = "spectral2", | ||
| direction_units: list[str] | None = None, | ||
| vel_unit: str = "km/s", | ||
| ) -> None: | ||
| """Normalise a CASA image coordinate dict for round-trip comparison. | ||
|
|
||
| When an image is written from an ``xr.Dataset`` and re-opened by | ||
| casacore, direction coordinate values are stored in radians whereas | ||
| the original CASA image stores them in arcminutes. This function | ||
| converts the direction entries in *coords* by multiplying by *factor* | ||
| (default: rad → arcmin) and sets the spectral velocity unit so the | ||
| two dicts can be compared with | ||
| :func:`~xradio.testing.assert_attrs_dicts_equal`. | ||
|
|
||
| Modifies *coords* **in place**. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| coords : dict | ||
| Coordinate dict returned by ``casacore.images.image.info()["coordinates"]`` | ||
| or ``casacore.tables.table.getkeywords()["coords"]``. | ||
| factor : float, optional | ||
| Multiplicative scale applied to ``cdelt`` and ``crval`` of the | ||
| direction sub-dict. Defaults to ``180 * 60 / π`` (radians → arcminutes). | ||
| direction_key : str, optional | ||
| Key of the direction coordinate entry in *coords*. | ||
| Defaults to ``"direction0"``. | ||
| spectral_key : str, optional | ||
| Key of the spectral coordinate entry in *coords*. | ||
| Defaults to ``"spectral2"``. | ||
| direction_units : list of str or None, optional | ||
| Unit strings written into the direction sub-dict after scaling. | ||
| Defaults to ``["'", "'"]`` (arcminutes) when *None*. | ||
| vel_unit : str, optional | ||
| Velocity unit string written into ``coords[spectral_key]["velUnit"]``. | ||
| Defaults to ``"km/s"``. | ||
| """ | ||
| if direction_units is None: | ||
| direction_units = ["'", "'"] | ||
| direction = coords[direction_key] | ||
| direction["cdelt"] *= factor | ||
| direction["crval"] *= factor | ||
| direction["units"] = direction_units | ||
| coords[spectral_key]["velUnit"] = vel_unit | ||
|
|
||
|
|
||
| def assert_image_block_equal( | ||
| xds: xr.Dataset, | ||
| output_path: str, | ||
| selection: Dict[str, slice], | ||
| zarr: bool = False, | ||
| do_sky_coords: bool = True, | ||
| ) -> None: | ||
| """Write an image, reload a spatial block, and assert equality with the | ||
| corresponding slice of the original dataset. | ||
|
|
||
| Workflow | ||
| -------- | ||
| 1. Write *xds* to *output_path*. | ||
| 2. Load the region specified by *selection* from the written image via | ||
| :func:`~xradio.image.load_image`. | ||
| 3. Compute the equivalent slice of *xds* with ``isel``. | ||
| 4. Assert equality using | ||
| :func:`~xradio.testing.assert_xarray_datasets_equal`. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| xds : xr.Dataset | ||
| Full image dataset to write and slice. Augment the dataset with | ||
| any extra data variables (e.g. ``BEAM_FIT_PARAMS``) *before* calling | ||
| this function if you want them included in the comparison. | ||
| output_path : str | ||
| Destination path for the written image. The path is overwritten if | ||
| it already exists. | ||
| selection : dict of str to slice | ||
| Mapping of dimension name to ``slice`` that defines the block to load | ||
| and compare. Every slice end must not exceed the corresponding | ||
| dimension size in *xds*. | ||
| zarr : bool, optional | ||
| If *True* write in zarr format; otherwise write as a CASA image. | ||
| Defaults to *False*. | ||
| do_sky_coords : bool, optional | ||
| Forwarded to :func:`~xradio.image.load_image` as ``do_sky_coords``. | ||
| Defaults to *True*. | ||
|
|
||
| Raises | ||
| ------ | ||
| ValueError | ||
| If any slice in *selection* exceeds the size of the corresponding | ||
| dimension in *xds*. | ||
| """ | ||
| from xradio.image import load_image, write_image | ||
| from xradio.testing import assert_xarray_datasets_equal | ||
|
|
||
| bad_dims = [] | ||
| for dim, slc in selection.items(): | ||
| size = xds.sizes.get(dim, 0) | ||
| stop = slc.stop if slc.stop is not None else size | ||
| if stop > size: | ||
| bad_dims.append(f"{dim}: slice stop {stop} > size {size}") | ||
| if bad_dims: | ||
| raise ValueError( | ||
| "assert_image_block_equal: selection exceeds dataset dimensions — " | ||
| + ", ".join(bad_dims) | ||
| ) | ||
|
|
||
| write_image(xds, output_path, out_format="zarr" if zarr else "casa", overwrite=True) | ||
|
|
||
| loaded = load_image(output_path, selection, do_sky_coords=do_sky_coords) | ||
| true_xds = xds.isel(**selection) | ||
| assert_xarray_datasets_equal(loaded, true_xds) | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| """Test-data generators for image tests. | ||
|
|
||
| All functions are framework-agnostic and can be used in pytest, ASV | ||
| benchmarks, or any other harness that imports ``xradio.testing.image``. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import numpy as np | ||
| import xarray as xr | ||
|
|
||
|
|
||
| def make_beam_fit_params(xds: xr.Dataset) -> xr.DataArray: | ||
| """Build a ``BEAM_FIT_PARAMS`` DataArray from an image Dataset. | ||
|
|
||
| Creates a synthetic beam-parameter array whose shape is derived from | ||
| the *time*, *frequency*, and *polarization* dimensions of *xds*. | ||
| The first time–channel–polarization entry is set to 2.0; all others | ||
| are 1.0. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| xds : xr.Dataset | ||
| An open image dataset with ``time``, ``frequency``, and | ||
| ``polarization`` dimensions. | ||
|
|
||
| Returns | ||
| ------- | ||
| xr.DataArray | ||
| Array with dims ``["time", "frequency", "polarization", | ||
| "beam_params_label"]`` and coords inherited from *xds*. | ||
| """ | ||
| shape = ( | ||
| xds.sizes["time"], | ||
| xds.sizes["frequency"], | ||
| xds.sizes["polarization"], | ||
| 3, | ||
| ) | ||
| ary = np.ones(shape, dtype=np.float32) | ||
| ary[0, 0, 0, :] = 2.0 | ||
| return xr.DataArray( | ||
| data=ary, | ||
| dims=["time", "frequency", "polarization", "beam_params_label"], | ||
| coords={ | ||
| "time": xds.time, | ||
| "frequency": xds.frequency, | ||
| "polarization": xds.polarization, | ||
| "beam_params_label": ["major", "minor", "pa"], | ||
| }, | ||
| ) | ||
|
|
||
|
|
||
| def create_empty_test_image(factory, do_sky_coords=None) -> xr.Dataset: | ||
| """Call a ``make_empty_*`` factory with canonical test arguments. | ||
|
|
||
| Provides a single set of standard test coordinates so every empty-image | ||
| factory can be exercised with the same call. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| factory : callable | ||
| One of ``make_empty_sky_image``, ``make_empty_aperture_image``, or | ||
| ``make_empty_lmuv_image``. | ||
| do_sky_coords : bool or None, optional | ||
| Forwarded as ``do_sky_coords`` keyword argument when not *None*. | ||
|
|
||
| Returns | ||
| ------- | ||
| xr.Dataset | ||
| The empty image dataset produced by *factory*. | ||
| """ | ||
| args = [ | ||
| [0.2, -0.5], # phase_center | ||
| [10, 10], # image_size | ||
| [np.pi / 180 / 60, np.pi / 180 / 60], # cell_size | ||
| [1.412e9, 1.413e9], # frequency | ||
| ["I", "Q", "U"], # polarization | ||
| [54000.1], # time | ||
| ] | ||
| kwargs = {} if do_sky_coords is None else {"do_sky_coords": do_sky_coords} | ||
| return factory(*args, **kwargs) | ||
|
|
||
|
|
||
| def scale_data_for_int16(data: np.ndarray) -> np.ndarray: | ||
| """Scale a float array to the int16 range for FITS BSCALE/BZERO testing. | ||
|
|
||
| Replaces NaNs with zero, clips to ``[-32768, 32767]``, and casts to | ||
| ``int16``. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| data : np.ndarray | ||
| Input floating-point array. | ||
|
|
||
| Returns | ||
| ------- | ||
| np.ndarray | ||
| A new array of dtype ``int16``. | ||
| """ | ||
| data = np.nan_to_num(data, nan=0.0) | ||
| data = np.clip(data, -32768, 32767) | ||
| return data.astype(np.int16) | ||
|
|
||
|
|
||
| def create_bzero_bscale_fits( | ||
| outname: str, source_fits: str, bzero: float, bscale: float | ||
| ) -> None: | ||
| """Write a FITS file with explicit BSCALE/BZERO headers for guard testing. | ||
|
|
||
| Reads pixel data from *source_fits*, scales it to the int16 range via | ||
| :func:`scale_data_for_int16`, and writes a new FITS primary HDU to | ||
| *outname* with the given BSCALE and BZERO header keywords. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| outname : str | ||
| Destination FITS file path. | ||
| source_fits : str | ||
| Source FITS file whose pixel data is used as the basis. | ||
| bzero : float | ||
| Value written to the ``BZERO`` header keyword. | ||
| bscale : float | ||
| Value written to the ``BSCALE`` header keyword. | ||
| """ | ||
| from astropy.io import fits | ||
|
|
||
| with fits.open(source_fits) as hdulist: | ||
| data = scale_data_for_int16(hdulist[0].data) | ||
| hdu = fits.PrimaryHDU(data=data) | ||
| hdu.header["BSCALE"] = bscale | ||
| hdu.header["BZERO"] = bzero | ||
| hdu.writeto(outname, overwrite=True) |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is implicitly for l,m (not u,v aperture) images only. Suggest name change to assert_sky_image_block_equal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmehring , the original code in test_image.py lines 410:415 are these:
Therefore the name given in the refactoring follows the intention of the original code. Do you still think we should rename it to assert_sky_image_block_equal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kept the same name, but generalized both functions. If renaming is still better, let me know and I will apply it.