Add a check for separation and tolerance in tweakreg

[mcara]

This PR adds a check that (abs_)separation > sqrt(2) * (abs_)tolerance input parameters to the tweakreg step. It also adds a comment in the docstring about these parameters.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

• added entry in CHANGES.rst within the relevant release section
• updated or added relevant tests
• updated relevant documentation
• added relevant milestone
• added relevant label(s)
• ran regression tests, post a link to the Jenkins job below.
  How to run regression tests on a PR
• All comments are resolved
• Make sure the JIRA ticket is resolved properly

[nden]

Agreed with Howard's comments. These need to be warnings.

[mcara]

I took into account Howard suggestion as well as (in part) @mairanteodoro suggestion in a different PR about putting common logging and step-skipping code into a single function.

[mcara]

Regression tests: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1445/

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.97%. Comparing base (4179c09) to head (4e69f2e).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8476      +/-   ##
==========================================
- Coverage   57.97%   57.97%   -0.01%     
==========================================
  Files         387      387              
  Lines       38830    38824       -6     
==========================================
- Hits        22513    22507       -6     
  Misses      16317    16317              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mcara]

Another regression test here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1447/

Previous test was failing because input models' meta could not be updated with info about step being skipped (models were not yet open)

and https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1448/

[nden]

@mcara About the common function for logging - #8424 refactors tweakreg to reduce memory. If this function prevents or complicate the refactoring, give preference to #8424 and remove it.

[mcara]

@mcara About the common function for logging - #8424 refactors tweakreg to reduce memory. If this function prevents or complicate the refactoring, give preference to #8424 and remove it.

@braingram what is your opinion on this change: 56a1108 ?

[braingram]

@braingram what is your opinion on this change: 56a1108 ?

Thanks for the ping. I will look at this later today. One general question, are these changes required for the check in the title of this PR?

[mcara]

One general question, are these changes required for the check in the title of this PR?

No. But... @mairanteodoro is right about similar code being used in too many places.

[braingram]

No. But... @mairanteodoro is right about similar code being used in too many places.

Perhaps a follow-up PR cleaning up the error handling and logging is called for? It could include an update to the tweakwcs pin once a version is released that contains spacetelescope/tweakwcs#203 (and possibly spacetelescope/tweakwcs#204)?

I rebased and reopened #8424 when this PR was only the parameter checks thinking it would have no conflicts. I don't think that's the case if the additional cleanup in 56a1108 is included. Since they're not required for the parameter changes I think they make sense to put in a different cleanup PR.

[mcara]

OK. I'll remove logging function and simplify this PR.

[mcara]

Just started https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1449/

[braingram]

Thanks for updating the PR. I added a few comments.

[mcara]

@hbushouse @nden regression tests fail with this error:

        # Check the status of the step is set correctly in the files.
        result = TweakRegStep.call(rtdata.input)
    
>       for fi in result._models:
E       AttributeError: 'builtin_function_or_method' object has no attribute '_models'

/data1/jenkins/workspace/RT/JWST-Developers-Pull-Requests/clone/jwst/regtest/test_niriss_image.py:65: AttributeError

Am I returning something incorrect or is this a recently added test that is setup in a different way that we had before. I need help with this. Thanks!

[mcara]

https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1450/

[mcara]

I think, @nden, these lines are causing an issue:

jwst/jwst/regtest/test_niriss_image.py

Lines 62 to 63 in 3df35ef

	# Check the status of the step is set correctly in the files.
	result = TweakRegStep.call(rtdata.input)

And since you already run the step above this call, I do not think this is necessary as the step should set skipped flag as @braingram mentioned above. You can just check it (lines below).

[mcara]

I made a PR that I hope will fix this test: #8489

[hbushouse]

@mcara Still need to address most recent comments and fix conflict that now exists in tweakreg_step.py

[mcara]

@hbushouse

@mcara Still need to address most recent comments and fix conflict that now exists in tweakreg_step.py

My apologies: somehow I thought I pushed latest changes last Friday.

[braingram]

Would you add a unit test (or 2) to verify the check. It would be helpful to avoid losing this in a refactor.

[braingram]

Thanks for adding the test. LGTM.

I started regtests with this PR here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1478/

[braingram]

@mcara the regtest run had 2 failures:

• test_nirspec_lamp_ifu_spec2 (unrelated, caused by fix in remove hard-coded reference file that uses grp path #8507)
• test_niriss_tweakreg_no_sources see below

    @pytest.mark.bigdata
    def test_niriss_tweakreg_no_sources(rtdata, fitsdiff_default_kwargs):
        """Make sure tweakreg is skipped when sources are not found.
        """
    
        rtdata.input = "niriss/imaging/jw01537-o003_20240406t164421_image3_00004_asn.json"
        rtdata.get_asn("niriss/imaging/jw01537-o003_20240406t164421_image3_00004_asn.json")
    
        args = [
            "jwst.tweakreg.TweakRegStep",
            rtdata.input,
            "--abs_refcat='GAIADR3'",
            "--save_results=True",
        ]
        result = Step.from_cmdline(args)
        # Check that the step is skipped
>       assert result.skip
E       assert False
E        +  where False = <jwst.tweakreg.tweakreg_step.TweakRegStep object at 0x7fde6dc1e4e0>.skip

Is this second one related to this PR?

[mcara]

Is this second one related to this PR?

Yes, see #8476 (comment)

[braingram]

Thanks. I attempted to dismiss my own review (since the regression test fails with this PR). However I didn't see a button to dismiss my own review so I re-requested myself as a reviewer. I didn't follow the test changes in #8450 #8477 and #8489 close enough to understand why it fails with this PR. If a review is helpful please feel free to ping me.

[hbushouse]

@mcara the regtest run had 2 failures:

• test_nirspec_lamp_ifu_spec2 (unrelated, caused by fix in remove hard-coded reference file that uses grp path #8507)
• test_niriss_tweakreg_no_sources see below

    @pytest.mark.bigdata
    def test_niriss_tweakreg_no_sources(rtdata, fitsdiff_default_kwargs):
        """Make sure tweakreg is skipped when sources are not found.
        """
    
        rtdata.input = "niriss/imaging/jw01537-o003_20240406t164421_image3_00004_asn.json"
        rtdata.get_asn("niriss/imaging/jw01537-o003_20240406t164421_image3_00004_asn.json")
    
        args = [
            "jwst.tweakreg.TweakRegStep",
            rtdata.input,
            "--abs_refcat='GAIADR3'",
            "--save_results=True",
        ]
        result = Step.from_cmdline(args)
        # Check that the step is skipped
>       assert result.skip
E       assert False
E        +  where False = <jwst.tweakreg.tweakreg_step.TweakRegStep object at 0x7fde6dc1e4e0>.skip

Is this second one related to this PR?

That test was likely failing because checking the value of result.skip is not a valid way to determine whether or not a test was skipped. We don't reset the value of the step's skip param on the fly when the code makes its own decision to skip/abort the step. It should only set the value of the cal_step status keyword to "SKIPPED" in the output data model. That's the proper way to test whether a step was skipped. Has this test been updated to account for that?

[hbushouse]

@mcara Now that this PR has removed the setting of self.skip=True when it decides to skip the step the regression test jwst/regtest/test_niriss_image.py::test_niriss_tweakreg_no_sources needs to be updated so that it no longer calls assert result.skip, because that will no longer be set to True when the step skips. The test should only check the returned values of model.meta.cal_step.tweakreg, as it's already doing.