Skip to content

Conversation

jemorrison
Copy link
Collaborator

@jemorrison jemorrison commented Apr 8, 2025

Resolves JP-3905

Closes #9240

This PR adds an option to the straylight step to save the shower model when also running the clean_shower option.
A new regression test running straylight with clean_showers and save_shower_model = True.
New regression Truth files were added to the regression test suite.

Tasks

@jemorrison jemorrison added this to the Build 12.0 milestone Apr 8, 2025
@jemorrison jemorrison requested a review from a team as a code owner April 8, 2025 19:36
@jemorrison
Copy link
Collaborator Author

Running the regression tests and I will add a test to compare the shower model

@jemorrison jemorrison force-pushed the JP-3905_save_shower_model branch from a6c0a93 to 92064f3 Compare April 8, 2025 19:39
@jemorrison
Copy link
Collaborator Author

Oh I need to probably update the docs to add this new parameter. I will do that.

@jemorrison jemorrison marked this pull request as draft April 8, 2025 19:43
Copy link

codecov bot commented Apr 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.58%. Comparing base (233ff95) to head (5ea9959).
Report is 678 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9378      +/-   ##
==========================================
+ Coverage   75.37%   75.58%   +0.21%     
==========================================
  Files         368      368              
  Lines       36885    36898      +13     
==========================================
+ Hits        27802    27890      +88     
+ Misses       9083     9008      -75     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jemorrison jemorrison force-pushed the JP-3905_save_shower_model branch from 92064f3 to 73f7985 Compare April 10, 2025 19:48
@jemorrison jemorrison marked this pull request as ready for review April 10, 2025 20:56
@jemorrison jemorrison requested a review from a team as a code owner April 10, 2025 20:56
@jemorrison
Copy link
Collaborator Author

running regression tests with new test.

@jemorrison
Copy link
Collaborator Author

Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

Testing locally, it looks like the file is saved as expected. Thanks for updating the docs and adding the new test as well!

It would be nice to add a unit test for the new output, as well as the regression test, to make sure it gets saved in the right location when output_dir is specified.

A few more comments and requests below.

Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

Checking in on your updates... A couple more clean up items remaining, and another follow up request for the regtest.

Looks like the unit test is still not working because: "Straylight correction not defined for datatype CubeModel". It will need a different synthetic input than the other unit test.

@jemorrison jemorrison force-pushed the JP-3905_save_shower_model branch from 3951f3b to b12e4d0 Compare April 15, 2025 16:27
@jemorrison
Copy link
Collaborator Author

@ re-running regression tests

Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

I'll rerun regtests one more time at the same link, but then I think this should be good to go.

Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

All good now!

@melanieclarke melanieclarke enabled auto-merge April 17, 2025 15:14
@melanieclarke melanieclarke merged commit 51debee into spacetelescope:main Apr 17, 2025
19 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.

Add option to write out residual shower model from straylight step
2 participants