Skip to content
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

JP-3686: Allow tweakreg to parse skycoord objects in absolute refcat #333

Merged
merged 4 commits into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/333.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow tweakreg to parse absolute reference catalogs with skycoord objects in them
32 changes: 31 additions & 1 deletion src/stcal/tweakreg/tweakreg.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import numpy as np
import math
import warnings
from pathlib import Path
Expand Down Expand Up @@ -253,14 +254,43 @@ def _parse_refcat(abs_refcat: str | Path,
)

if Path(abs_refcat).is_file():
return Table.read(abs_refcat)
return _parse_sky_centroid(Table.read(abs_refcat))

msg = (f"Invalid 'abs_refcat' value: {abs_refcat}. 'abs_refcat' must be "
"a path to an existing file name or one of the supported "
f"reference catalogs: {_SINGLE_GROUP_REFCAT_STR}.")
raise ValueError(msg)


def _parse_sky_centroid(catalog: Table) -> Table:
"""Turn SkyCoord object into simple RA/DEC columns.

The inclusion of SkyCoord objects via sky_centroid.ra and sky_centroid.dec
permits the use of catalogs directly from the jwst source_catalog step.
No action is taken if the catalog already contains RA and DEC columns.
"""
cols = [name.lower() for name in catalog.colnames]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a table contain RA, rA, Ra, and ra columns at the same time? If "Yes", then I think this code should be enhanced and throw an exception if there are multiple instances of ra in cols list. While it may never happen, we must catch something like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, maybe implement a logic like: if RA and DEC are present - use those, if not try ra and dec, if not use lower() and try again...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good suggestion, this is changed in the most recent version. I think it's best for the step to fail if it sees e.g. RA and ra, so that the user doesn't accidentally get unexpected results from the wrong column.

if ("ra" in cols) and ("dec" in cols):
if "sky_centroid" in cols:
msg = ("Catalog contains both (RA, DEC) and sky_centroid. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it contains both (RA, DEC) or maybe contains both (ra, dEc)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the error message a bit, let me know if you are ok with the change

"Ignoring sky_centroid.")
warnings.warn(msg, stacklevel=2)
catalog.remove_column("sky_centroid")
return catalog
if "sky_centroid" not in cols:
msg = ("Absolute reference catalog contains neither RA, DEC "
"nor sky_centroid.ra, sky_centroid.dec.")
raise KeyError(msg)

skycoord = catalog["sky_centroid"].to_table()

catalog["ra"] = skycoord["ra"]
catalog["dec"] = skycoord["dec"]
catalog.remove_column("sky_centroid")

return catalog


def _is_wcs_correction_small(correctors,
use2dhist=True,
searchrad=2.0,
Expand Down
48 changes: 41 additions & 7 deletions tests/test_tweakreg.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@
from astropy.modeling.models import Shift
from astropy.table import Table
from astropy.time import Time
from astropy.coordinates import SkyCoord

from stcal.tweakreg import astrometric_utils as amutils
from stcal.tweakreg.tweakreg import (
TweakregError,
_is_wcs_correction_small,
_parse_refcat,
_parse_sky_centroid,
_wcs_to_skycoord,
absolute_align,
construct_wcs_corrector,
Expand Down Expand Up @@ -203,17 +205,19 @@ def datamodel(wcsobj2, group_id=None):
return MinimalDataWithWCS(wcsobj2, group_id=group_id)


def test_parse_refcat(datamodel, tmp_path):
@pytest.fixture(scope="module")
def abs_refcat(datamodel):

wcsobj = datamodel.meta.wcs
correctors = fake_correctors(0.0)

# Get radius and fiducial
radius, fiducial = amutils.compute_radius(wcsobj)
return amutils.get_catalog(fiducial[0], fiducial[1], search_radius=radius,
catalog=TEST_CATALOG)

# Get the catalog
cat = amutils.get_catalog(fiducial[0], fiducial[1], search_radius=radius,
catalog=TEST_CATALOG)

def test_parse_refcat(datamodel, abs_refcat, tmp_path):

correctors = fake_correctors(0.0)
cat = abs_refcat

# save refcat to file
cat.write(tmp_path / CATALOG_FNAME, format="ascii.ecsv", overwrite=True)
Expand All @@ -232,6 +236,36 @@ def test_parse_refcat(datamodel, tmp_path):
assert isinstance(refcat, Table)


def test_parse_sky_centroid(abs_refcat):

# make a SkyCoord object out of the RA and DEC columns
cat = abs_refcat.copy()
sky_centroid = SkyCoord(cat["ra"], cat["dec"], unit="deg")
cat["sky_centroid"] = sky_centroid

# test case where ra, dec, and sky_centroid are all present
with pytest.warns(UserWarning):
cat_out = _parse_sky_centroid(cat)
cat_out = _parse_sky_centroid(cat)
assert isinstance(cat_out, Table)
assert np.all(cat["ra"] == cat_out["ra"])
assert np.all(cat["dec"] == cat_out["dec"])
assert "sky_centroid" not in cat_out.columns

# test case where ra, dec are no longer present
cat["sky_centroid"] = sky_centroid
cat.remove_columns(["ra", "dec"])
cat_out = _parse_sky_centroid(cat)
assert isinstance(cat_out, Table)
assert np.all(cat["ra"] == cat_out["ra"])
assert np.all(cat["dec"] == cat_out["dec"])

# test case where neither present
cat.remove_columns(["ra", "dec"])
with pytest.raises(KeyError):
_parse_sky_centroid(cat)


@pytest.fixture(scope="module")
def input_catalog(datamodel):
"""Get catalog from gaia, transform it to x,y in the image frame,
Expand Down
Loading