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

TweakReg regression test fix. #919

Merged

Conversation

mairanteodoro
Copy link
Collaborator

@mairanteodoro mairanteodoro commented Oct 2, 2023

This PR implements the following items to fix TweakReg's (TR) regression test:

  • fix TR's catalog metadata: a bug was preventing the catalog metadata name from being properly set;
  • updated the files used by TweakReg's regression test:
    • input: roman-pipeline/dev/WFI/image/r0000501001001001001_01101_0001_WFI02_cal_tweakreg.asdf;
    • truth: roman-pipeline/dev/truth/WFI/image/r0000501001001001001_01101_0001_WFI02_tweakreg.asdf.

The truth file in Artifactory was created by processing the cal file through the TweakRegStep:

from romancal.stpipe import RomanStep
from romancal.tweakreg.tweakreg_step import TweakRegStep

step = TweakRegStep()

args = [
    "romancal.step.TweakRegStep",
    "/full/path/to/association/file/asn_filename.json",
    "--output_file='tweakreg_truth_filename.asdf'",
    "--suffix='output'",
]
RomanStep.from_cmdline(args)

The association file (named asn_filename.json above) used in the creation of the truth file has the following content:

{
       "asn_type": "None",
       "asn_rule": "DMS_ELPP_Base",
       "version_id": null,
       "code_version": "0.9.1.dev28+ge987cc9.d20230106",
       "degraded_status": "No known degraded exposures in association.",
       "program": "noprogram",
       "constraints": "No constraints",
       "asn_id": "a3001",
       "target": "none",
       "asn_pool": "test_pool_name",
       "products": [
           {
               "name": "files.asdf",
               "members": [
                   {
                       "expname": "r0000501001001001001_01101_0001_WFI02_cal_tweakreg.asdf",
                       "exptype": "science"
                   }
               ]
            }
       ]
}

The file used as input for the regression test, r0000501001001001001_01101_0001_WFI02_cal.asdf, was provided by @tddesjardins and has been processed through assign_wcs, photom, and source_detection. We added the tweakreg suffix before uploading it to Artifactory.

Astrometric Utils unit tests
This PR also adds parallax correction to the unit tests for the astrometric_utils package.

Since the coordinates in the catalog obtained from the VO API are now being updated for proper motion and parallax, we must consider that in the test_astrometric_utils suite.

Regression test
All regression tests are passing: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/401/
${\text{\color{red}The regression tests are failing to start and Zach and William are working on fixing it.}}$

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
romancal/tweakreg/tweakreg_step.py 90.68% <100.00%> (-0.04%) ⬇️

📢 Thoughts on this report? Let us know!.

@ddavis-stsci
Copy link
Collaborator

Is there some reason you are switching from WFI01 to WFI02?
Does this mean that we need to retire the files for the older tests once this is in?
The WFI02 file is also a custom file from INS (Tyler)?

@mairanteodoro
Copy link
Collaborator Author

mairanteodoro commented Oct 3, 2023

Is there some reason you are switching from WFI01 to WFI02?

Yes; the reason is that seven files are throwing an error (ValueError: Out of domain for acos) when calculating the area of the spherical triangles during the TweakRegStep processing. The files are:

  • r0000501001001001001_01101_0001_WFI07_cal.asdf;
  • r0000501001001001001_01101_0001_WFI16_cal.asdf;
  • r0000501001001001001_01101_0001_WFI17_cal.asdf;
  • r0000501001001001001_01101_0002_WFI01_cal.asdf;
  • r0000501001001001001_01101_0002_WFI03_cal.asdf;
  • r0000501001001001001_01101_0002_WFI09_cal.asdf;
  • r0000501001001001001_01101_0002_WFI10_cal.asdf.

If we can't process the files through TweakRegStep, we can't pass it to ResampleStep. Both @tddesjardins and @nden are aware of this issue.

For the resample regression test specifically, we are using SCA 02 because that allows us to test the alignment of the same SCA but from different exposures (the latest simulations have a small offset between exposures). SCA 01 can't be used for that since it can't be processed through TweakRegStep.

Does this mean that we need to retire the files for the older tests once this is in?

I don't think so; this issue affects only TweakReg.

The WFI02 file is also a custom file from INS (Tyler)?

Yes, it is a WFISim file with Gaia sources in it.

@mairanteodoro mairanteodoro force-pushed the tweakreg_regression_test_fix branch from b8f9f66 to f873f7b Compare October 3, 2023 15:11
@ddavis-stsci
Copy link
Collaborator

Do we have a timescale of when the "error (ValueError: Out of domain for acos)" might be fixed? This clearly cannot go into the elpp until this is working.
At that point we should revert the file to WFI01, or convert all the tests to use WFI02.

@mairanteodoro
Copy link
Collaborator Author

Do we have a timescale of when the "error (ValueError: Out of domain for acos)" might be fixed? This clearly cannot go into the elpp until this is working. At that point we should revert the file to WFI01, or convert all the tests to use WFI02.

The ticket for that work is https://jira.stsci.edu/browse/RCAL-688.

@mairanteodoro mairanteodoro force-pushed the tweakreg_regression_test_fix branch from 61389a5 to 94acb0a Compare October 5, 2023 15:00
@mairanteodoro mairanteodoro requested a review from schlafly October 5, 2023 18:28
@schlafly
Copy link
Collaborator

schlafly commented Oct 5, 2023

What is the actual level of agreement you're getting?

I said atol = 0.001 but I'm not sure what I was thinking; that's the 3.6" agreement we were talking about before. You did atol=1e-5, which is 0.036" = 0.36 pixel, which is not great; we really want ~0.01 - 0.001 pix or something.

@mairanteodoro mairanteodoro force-pushed the tweakreg_regression_test_fix branch from b94dc4a to 2566277 Compare October 11, 2023 23:43
@mairanteodoro
Copy link
Collaborator Author

mairanteodoro commented Oct 12, 2023

What is the actual level of agreement you're getting?

I said atol = 0.001 but I'm not sure what I was thinking; that's the 3.6" agreement we were talking about before. You did atol=1e-5, which is 0.036" = 0.36 pixel, which is not great; we really want ~0.01 - 0.001 pix or something.

~We are using atol=1e-8 (absolute coordinates difference, in deg) in the unit tests, which corresponds to an absolute difference in coordinates of $\lessapprox 40$ μas (0.0004 pix). See comment below.

@mairanteodoro
Copy link
Collaborator Author

mairanteodoro commented Oct 12, 2023

The tolerance for the difference between the expectation and the catalog updated coordinates has been set to $2.8\times10^{-9}$ deg, corresponding to 10 μas. Also, instead of using a one-to-one comparison, we filter the differences by the median.

Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

Thanks Mairan. That looks good to me.

@mairanteodoro mairanteodoro merged commit 9d59ab7 into spacetelescope:main Oct 12, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants