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

RCAL-698: fix TweakReg to allow for Roman datamodels as input #1089

Conversation

mairanteodoro
Copy link
Collaborator

@mairanteodoro mairanteodoro commented Feb 2, 2024

Resolves RCAL-698

This PR addresses code refactoring the TweakRegStep to allow Roman datamodels to be passed in to TweakReg.

Regression test results
All regression tests are successful (ran on Mar 4th): https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/624/

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

@github-actions github-actions bot added documentation Improvements or additions to documentation testing pipeline labels Feb 2, 2024
@mairanteodoro mairanteodoro changed the title RCAL-698: fix TweakReg to allow for Roman datamodels input format RCAL-698: fix TweakReg to allow for Roman datamodels as input Feb 2, 2024
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.72%. Comparing base (046faae) to head (795f99d).
Report is 2 commits behind head on main.

❗ Current head 795f99d differs from pull request most recent head 187b864. Consider uploading reports for the commit 187b864 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1089      +/-   ##
==========================================
- Coverage   76.73%   76.72%   -0.02%     
==========================================
  Files         105      105              
  Lines        7054     7054              
==========================================
- Hits         5413     5412       -1     
- Misses       1641     1642       +1     
Flag Coverage Δ *Carryforward flag
nightly 62.72% <ø> (-0.02%) ⬇️ Carriedforward from f354e98

*This pull request uses carry forward flags. Click here to find out more.

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

@mairanteodoro mairanteodoro marked this pull request as ready for review February 15, 2024 20:14
@mairanteodoro mairanteodoro requested a review from a team as a code owner February 15, 2024 20:14
@ddavis-stsci
Copy link
Collaborator

I'm not sure that this is doing what we need. It does accept a model container but it seems to run tweakreg on each input in a sequence.

I thought that the plan is to run tweakreg on all the input files in the association as a group so that the final solution is more accurate.

@mairanteodoro
Copy link
Collaborator Author

I'm not sure that this is doing what we need. It does accept a model container but it seems to run tweakreg on each input in a sequence.

I thought that the plan is to run tweakreg on all the input files in the association as a group so that the final solution is more accurate.

Ticket RCAL-698 asks for a fix to allow TweakReg to accept a Roman datamodel as input. Below is an excerpt from the description section of the ticket:

Screenshot 2024-02-16 at 3 19 19 PM

If you pass a ModelContainer, an association filename, or a list of Roman datamodels (or ASDF files), then TweakReg will process everything just as you mentioned. This PR addresses only what's in the ticket.

@github-actions github-actions bot removed the pipeline label Feb 20, 2024
docs/roman/tweakreg/tweakreg_examples.rst Outdated Show resolved Hide resolved
docs/roman/tweakreg/tweakreg_examples.rst Outdated Show resolved Hide resolved
docs/roman/tweakreg/tweakreg_examples.rst Outdated Show resolved Hide resolved
romancal/tweakreg/tests/test_tweakreg.py Outdated Show resolved Hide resolved
romancal/tweakreg/tests/test_tweakreg.py Outdated Show resolved Hide resolved
@nden
Copy link
Collaborator

nden commented Feb 21, 2024

I left inline questions that I think need to be answered. A bit more generally the input/output support in Tweakreg needs to be

  1. Association file (string) in a Python session and on the command line --> returns a ModelContainer (saves files to disk)
  2. A list of file names or datamodels (from Python) --> returns a ModelContainer (saves files to disk)
  3. A ModelContainer --> returns a ModelContainer (saves files to disk)
  4. Single image (string filename and datamodel) in Python and cmd --> returns single image (saved to disk)

Some of these won't work and will need added support in datamodels but that can be added in a different PR. For example opening an association file with datamodels.open fails for me.

@mairanteodoro
Copy link
Collaborator Author

mairanteodoro commented Feb 29, 2024

I left inline questions that I think need to be answered.

Done.

A bit more generally the input/output support in Tweakreg needs to be

  1. Association file (string) in a Python session and on the command line --> returns a ModelContainer (saves files to disk)
  2. A list of file names or datamodels (from Python) --> returns a ModelContainer (saves files to disk)
  3. A ModelContainer --> returns a ModelContainer (saves files to disk)
  4. Single image (string filename and datamodel) in Python and cmd --> returns single image (saved to disk)

Items 1, 2, and 3 are currently supported. As you mentioned, we can implement item 4 later on.


.. code-block:: python

from romancal.tweakreg.tweakreg_step import TweakRegStep
step = TweakRegStep()
step.process([img])
step([img])
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are advertising using Step.call because this guarantees using all parameters and avoids confusion. The examples should read

 output = TweakRegStep.call(...)

As written they use Step.run which instantiates a new step and ignores any parameters users pass.

with pytest.raises(Exception) as exec_info:
trs.TweakRegStep.call(input)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this is the correct signature to use.

Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

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

I am approving the PR but the documentation needs to be fixed before merging.
And it needs another regression test run.

@mairanteodoro mairanteodoro merged commit 030ef1d into spacetelescope:main Mar 5, 2024
26 checks passed
@mairanteodoro
Copy link
Collaborator Author

I am approving the PR but the documentation needs to be fixed before merging. And it needs another regression test run.

Done and done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Photom Saturation testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants