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

Attention: Patch coverage is 28.57143% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 57.96%. Comparing base (781e0e0) to head (0516436).
Report is 12 commits behind head on master.

Files	Patch %	Lines
jwst/tweakreg/tweakreg_step.py	28.57%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8476      +/-   ##
==========================================
+ Coverage   57.93%   57.96%   +0.03%     
==========================================
  Files         387      387              
  Lines       38839    38844       +5     
==========================================
+ Hits        22502    22517      +15     
+ Misses      16337    16327      -10     

☔ 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.