-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
@mcara this isn't ready for formal review yet, but I'm wondering whether you'd like this change (or a version of it) in tweakwcs instead, or if here is the best place for it in your opinion. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #333 +/- ##
===========================================
+ Coverage 29.57% 86.99% +57.41%
===========================================
Files 36 49 +13
Lines 7949 9012 +1063
===========================================
+ Hits 2351 7840 +5489
+ Misses 5598 1172 -4426 ☔ View full report in Codecov by Sentry. |
Regression tests for jwst started here: https://github.com/spacetelescope/RegressionTests/actions/runs/12911371129 Regression tests for romancal started here: https://github.com/spacetelescope/RegressionTests/actions/runs/12911410864 Failing downstream unit tests for romancal appear to be unrelated. |
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] |
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.
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.
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.
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...
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.
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.
src/stcal/tweakreg/tweakreg.py
Outdated
cols = [name.lower() for name in catalog.colnames] | ||
if ("ra" in cols) and ("dec" in cols): | ||
if "sky_centroid" in cols: | ||
msg = ("Catalog contains both (RA, DEC) and sky_centroid. " |
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.
is it contains both (RA, DEC)
or maybe contains both (ra, dEc)
?
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.
I updated the error message a bit, let me know if you are ok with the change
I prefer to keep things simple. It is documented what |
In that case, I think this is the best place for it. I think changing the output format of the source catalog step would be more disruptive, e.g., if users have already written workflows that ingest this information. Changing the columns in the output to source catalog would also make it less Astropy-integrated, which isn't necessarily desired either. This is a special case and should be the only "custom" format we need to support. |
Another suggestion for rare cases such as this and maybe setting sky background, etc. is to have |
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.
Looks good to me - happy to see our steps work together more coherently.
Resolves JP-3686
Closes spacetelescope/jwst#8639
This PR allows the tweakreg step to parse absolute reference catalogs that resemble the output from the JWST
source_catalog
step. That step reports absolute RA, DEC in columns calledsky_centroid.ra
andsky_centroid.dec
which are parsed by into an astropy Table as a single column containing a SkyCoord. This PR adds the functionality to re-cast this into RA/DEC columns as expected bytweakwcs.align_wcs
.Tasks
docs/
pageno-changelog-entry-needed
)changes/
:echo "changed something" > changes/<PR#>.<changetype>.rst
(see below for change types)"git+https://github.com/<fork>/stcal@<branch>"
)jwst
regression testromancal
regression testnews fragment change types...
changes/<PR#>.apichange.rst
: change to public APIchanges/<PR#>.bugfix.rst
: fixes an issuechanges/<PR#>.general.rst
: infrastructure or miscellaneous change