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.

Sign up for GitHub

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

Jump to bottom

rcal-700 Update exposure pipeline to correct cal_step and suffixes #971

Merged
merged 4 commits into from
Nov 3, 2023
Merged

rcal-700 Update exposure pipeline to correct cal_step and suffixes #971

merged 4 commits into from
Nov 3, 2023

Conversation

ddavis-stsci
Copy link
Collaborator

Resolves RCAL-700

Closes #962

This PR
For SDF the first stage of the pipeline will run and produce

run_elpp.csh test_asn.json
...
2023-11-03 10:02:28,955 - stpipe.ExposurePipeline.tweakreg - INFO - Step skipped.
2023-11-03 10:02:28,956 - stpipe.ExposurePipeline.tweakreg - INFO - Step tweakreg done
2023-11-03 10:02:28,956 - stpipe.ExposurePipeline - INFO - Roman exposure calibration pipeline ending...
2023-11-03 10:02:32,525 - stpipe.ExposurePipeline - INFO - Saved model in r0000101001001001001_01101_0001_WFI01_sourcedetection.asdf
2023-11-03 10:02:35,976 - stpipe.ExposurePipeline - INFO - Saved model in r0000101001001001001_01101_0001_WFI02_sourcedetection.asdf
2023-11-03 10:02:38,760 - stpipe.ExposurePipeline - INFO - Saved model in r0000101001001001001_01101_0001_WFI03_sourcedetection.asdf
2023-11-03 10:02:38,760 - stpipe.ExposurePipeline - INFO - Step ExposurePipeline done

with the cal_step for tweakreg set to
tweakreg SKIPPED
The second step will run tweakreg on the individual files produced and yield _cal files

run_tweakreg.csh test_tweakreg_asn.json
...
2023-11-03 10:21:09,236 - stpipe.TweakRegStep - INFO - Saved model in r0000101001001001001_01101_0001_WFI01_cal.asdf
2023-11-03 10:21:13,649 - stpipe.TweakRegStep - INFO - Saved model in r0000101001001001001_01101_0001_WFI02_cal.asdf
2023-11-03 10:21:18,120 - stpipe.TweakRegStep - INFO - Saved model in r0000101001001001001_01101_0001_WFI03_cal.asdf
2023-11-03 10:21:18,120 - stpipe.TweakRegStep - INFO - Step TweakRegStep done

with
with the cal_step for tweakreg set to
tweakreg COMPLETE

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • [N/A] updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)
  • [N/A] ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR
    Regression testing is currently broken, local tests pass

@ddavis-stsci ddavis-stsci added this to the 24Q1_B12 milestone Nov 3, 2023
@ddavis-stsci ddavis-stsci self-assigned this Nov 3, 2023
@ddavis-stsci ddavis-stsci requested a review from a team as a code owner November 3, 2023 14:39
@codecov Codecov
Copy link

codecov bot commented Nov 3, 2023 •
edited
Loading

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Files Coverage Δ
romancal/lib/suffix.py 94.28% <ø> (ø)
romancal/pipeline/exposure_pipeline.py 30.07% <0.00%> (-7.22%) ⬇️

📢 Thoughts on this report? Let us know!.

schlafly
schlafly approved these changes Nov 3, 2023
View reviewed changes
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.

Looks good to me. There were a couple of lines that surprised me that I noted inline.

romancal/pipeline/exposure_pipeline.py Outdated
@@ -171,8 +174,12 @@ def process(self, input):
result.meta.cal_step.photom = "SKIPPED"
result.meta.cal_step.source_detection = "SKIPPED"
result.meta.cal_step.tweakreg = "SKIPPED"
self.suffix = "COMPLETE"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks weird? Do we really want to set the file name suffix to COMPLETE?

romancal/pipeline/exposure_pipeline.py
if file_type == "asn":
result = self.tweakreg(tweakreg_input)
for model in result._models:
model.meta.cal_step.tweakreg = "SKIPPED"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I expected COMPLETE here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When tweakreg is run with the option "--steps.tweakreg.skip=True" it is not honoring that and setting the step to skipped. So for the test RCAL-665 to pass this needs to be manually set to "SKIPPED".

This should not remain in place. My plan is to write a function that will check the skip flag that is passed to the step and set it appropriately and use this for all the pipeline steps. I think we should have a uniform method. I checked JWST and they seem to handle this flag in a non-uniform manner and I don't want to reproduce that. I'd like to fix this after B12.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, thanks.

@ddavis-stsci ddavis-stsci merged commit 6b6c3ec into spacetelescope:main Nov 3, 2023
@ddavis-stsci ddavis-stsci deleted the rcal-700_dsd branch November 3, 2023 19:32
ddavis-stsci added a commit to ddavis-stsci/romancal that referenced this pull request Nov 7, 2023
@ddavis-stsci @pre-commit-ci
rcal-700 Update exposure pipeline to correct cal_step and suffixes (s…
af0a7ca 
…pacetelescope#971)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schlafly schlafly schlafly approved these changes

Assignees

@ddavis-stsci ddavis-stsci

Labels
pipeline
Projects
None yet
Milestone
24Q1_B12
Development

Successfully merging this pull request may close these issues.

Update elpp for tweakreg returning an empty modelcontainer
2 participants
@ddavis-stsci @schlafly