RCAL-895: allow updating source catalog with tweaked WCS when running ELP #1373
Conversation
mairanteodoro
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1373 +/- ##
==========================================
+ Coverage 78.52% 78.57% +0.05%
==========================================
Files 117 117
Lines 7842 7857 +15
==========================================
+ Hits 6158 6174 +16
+ Misses 1684 1683 -1
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…tweaked-astrometry
| @@ -71,6 +71,8 @@ def process(self, input): | |||
|
|
|||
| # make sure source_catalog returns the updated datamodel | |||
| self.source_catalog.return_updated_model = True | |||
| # make sure we update source catalog coordinates afer running TweakRegStep | |||
| self.tweakreg.update_source_catalog_coordinates = True | |||
There was a problem hiding this comment.
It looks like this attribute is set here and only here. Patching the step object like this is not a good idea.
It should be added to the step spec with a default value.
There was a problem hiding this comment.
Mairan, Nadia and I chatted briefly about this earlier today, and I had gotten confused with the related self.source_catalog.return_updated_model above. Let's make this default False for now, but it could go into the spec I think?
github-actions
bot
added
the
dependencies
…tweaked-astrometry
…tweaked-astrometry
| assert not filecmp.cmp("img_1_cat.asdf", "img_1_cat_original.asdf") | ||
| # compare coordinates using rtol=1e-07 and atol=0 (default) | ||
| np.testing.assert_allclose( | ||
| cat_original.source_catalog["ra_centroid"], | ||
| cat_updated.source_catalog["ra_centroid"], | ||
| ) | ||
| np.testing.assert_allclose( | ||
| cat_original.source_catalog["dec_centroid"], | ||
| cat_updated.source_catalog["dec_centroid"], | ||
| ) | ||
| np.testing.assert_allclose( | ||
| cat_original.source_catalog["ra_psf"], | ||
| cat_updated.source_catalog["ra_psf"], | ||
| ) | ||
| np.testing.assert_allclose( | ||
| cat_original.source_catalog["dec_psf"], | ||
| cat_updated.source_catalog["dec_psf"], | ||
| ) |
There was a problem hiding this comment.
Can you help me with this test? The test is that the files are not bitwise identical, but the ras and decs didn't change meaningfully? I was kind of expecting this test to show that the ras and decs do change, and I read this test as showing the opposite; am I missing something?
I don't know what precision works, but I'd drop the filecmp.cmp test, and maybe make the tests look like:
for column in [ra, dec, centroid, psf]:
assert np.allclose(orig.catalog[column], new.catalog[column], atol=1/60/60) # assert nothing moved by more than an arcsecond
assert not np.allclose(orig.catalog[column], new.catalog[column], atol=1/60/60/100) # assert some things moved by at least a hundredth of an arcsecond
I'm kind of guessing what the right precisions are---it depends on how many stars are in the catalog, and how much you shifted their positions. But we want to be showing that we did tweak the positions, and I feel like the current test is kind of showing the opposite?
Note 1e-7 rtol corresponds to roughly 0.1" or 1 pixel. Do you know how big of a tweak you're applying?
Resolves RCAL-895
This PR implements the update of the source catalog coordinates using the improved WCS calculated by
TweakRegStepwhen processing data through the ELP.When running the ELP, at the end of the
TweakRegStep, thecatfile produced bySourceCatalogStep(containing all the sources coordinates) will be read in, updated with the new WCS function, and saved back to disk (overwriting the file content).Regression tests
All tests are passing: https://github.com/spacetelescope/RegressionTests/actions/runs/10892193910
Checklist
CHANGES.rstunder the corresponding subsection