-
Notifications
You must be signed in to change notification settings - Fork 171
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
JP-3733 remove setting pixel_replace.save to True in calwebb_spec3 #8765
JP-3733 remove setting pixel_replace.save to True in calwebb_spec3 #8765
Conversation
@tapastro Working on trying to figure out how to run regression tests in new system |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8765 +/- ##
=======================================
Coverage 61.75% 61.76%
=======================================
Files 377 377
Lines 38749 38742 -7
=======================================
- Hits 23931 23929 -2
+ Misses 14818 14813 -5 ☔ View full report in Codecov by Sentry. |
jwst/pipeline/calwebb_spec3.py
Outdated
|
||
# Overriding the Step.save_model method for the following steps. | ||
# These steps save intermediate files, resulting in meta.filename | ||
# being modified. This can affect the filenames of subsequent | ||
# steps. | ||
self.outlier_detection.save_model = invariant_filename(self.outlier_detection.save_model) | ||
self.pixel_replace.save_model = invariant_filename(self.pixel_replace.save_model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line still needed? Say, if the user manually sets save_results = True, will the subsequent output filenames be affected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find any difference in the filenames (when pixel_replace.save_results=True) in having this line and or removed.
Since it is not doing any harm maybe we should leave it. Removing it may have unintended effects.
I will add it back in
5d2c37b
to
4e5d94b
Compare
4e5d94b
to
9aef412
Compare
@melanieclarke @tapastro |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Testing locally on MIRI LRS data, which runs pixel_replace by default, no file is saved by default. Manually setting save_results=True for pixel_replace does save the file as expected.
Resolves JP-3733
Closes #8754
This PR addresses removes saving the pixel_replace step output by default. The user must set pixel_replace.save_results=True to get the output written to disk.
Tasks
Build 11.3
(use the latest build if not sure)add an entry to
CHANGES.rst
within the relevant release section (otherwise add theno-changelog-entry-needed
label to this PR)update or add relevant tests
update relevant docstrings and / or
docs/
pagestart a regression test and include a link to the running job (click here for instructions)
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1719/
okify_regtests
to update the truth files