-
Notifications
You must be signed in to change notification settings - Fork 174
JP-2645 - Snowball and Shower flagging in Jump #7039
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
Conversation
I can't figure out why the unit tests are failing. They work for me. The assert error is saying something about file (s) not being closed. It is referring to the gain and read noise files. |
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.
Overall looks good. Just minor comments about wording and typos.
I see that you added some tests in stcal for some of the functions you added there, but I think it would be good to have a few tests in JWST as well for the full step using these new options |
2eebfb2
to
75fe70d
Compare
@mwregan2 (summarizing some of our call earlier) i added a test to demonstrate the use of the for now i have commented this test out |
Codecov ReportBase: 79.51% // Head: 79.66% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #7039 +/- ##
==========================================
+ Coverage 79.51% 79.66% +0.14%
==========================================
Files 412 412
Lines 37322 37356 +34
==========================================
+ Hits 29678 29758 +80
+ Misses 7644 7598 -46
*This pull request uses carry forward flags. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
66c226c
to
7176b5a
Compare
7176b5a
to
1030e8f
Compare
I'm assuming that there's no real benefit in running a regtest against the PR branch, because the new stuff is turned off by default, hence we don't expect the new code to get exercised at all. Given that, I'm thinking we're ready to merge. Any objections? |
Yes, this will be transparent until we add the parameter reference files. |
Full regression test run on the master branch after merging this PR was clean, as expected (no changes). |
The tests were failing because files were not being closed. I just added the close to get the existing tests to work. I have no idea what changed to require that files be closed.
From: Clare Shanahan ***@***.***>
Reply-To: spacetelescope/jwst ***@***.***>
Date: Wednesday, October 5, 2022 at 3:28 PM
To: spacetelescope/jwst ***@***.***>
Cc: Michael Regan ***@***.***>, Author ***@***.***>
Subject: Re: [spacetelescope/jwst] JP-2645 - Snowball and Shower flagging in Jump (PR #7039)
@cshanahan1 commented on this pull request.
________________________________
In jwst/jump/tests/test_jump_step.py<https://urldefense.com/v3/__https:/github.com/spacetelescope/jwst/pull/7039*discussion_r988270460__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!21jzkLU4neZF2_rgqTv6-tJwerwwXgFElihdguVOHjE7KStWNiepU8Uu8DTxuxKvxBB3gAp7Nu1_FuWPvXnA698$>:
@@ -27,6 +27,7 @@ def generate_miri_reffiles(tmpdir_factory):
gain_model.meta.subarray.xsize = xsize
gain_model.meta.subarray.ysize = ysize
gain_model.save(gainfile)
+ gain_model.close()
they are written out to one of the pytest directories. i think you can override a reference file with a datamodel instead of a file path right, so these wouldn't have to be written out? i can open another pr to change this if thats the case
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/spacetelescope/jwst/pull/7039*discussion_r988270460__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!21jzkLU4neZF2_rgqTv6-tJwerwwXgFElihdguVOHjE7KStWNiepU8Uu8DTxuxKvxBB3gAp7Nu1_FuWPvXnA698$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AF6KJLXLNYIJNIF3EV53Y3LWBXJFBANCNFSM6AAAAAAQJ7T5SA__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!21jzkLU4neZF2_rgqTv6-tJwerwwXgFElihdguVOHjE7KStWNiepU8Uu8DTxuxKvxBB3gAp7Nu1_FuWPG9HC0KA$>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Resolves JP-2645
Add parameters to allow flagging of snowballs and showers. Corresponding PR in stcal is spacetelescope/stcal#118
Checklist for maintainers
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR