JP-3827: Remove unused error from ramp data #334
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #334 +/- ##
==========================================
+ Coverage 85.50% 86.16% +0.65%
==========================================
Files 50 50
Lines 9398 9322 -76
==========================================
- Hits 8036 8032 -4
+ Misses 1362 1290 -72 ☔ View full report in Codecov by Sentry. |
melanieclarke
force-pushed
the
jp-3827
branch
from
3c24bdd to
681136b
Compare
github-actions
bot
added
the
documentation
|
jwst tests here: https://github.com/spacetelescope/RegressionTests/actions/runs/13039620799 jwst tests show expected changes, romancal tests pass. |
melanieclarke
force-pushed
the
jp-3827
branch
from
def6691 to
61c6ea0
Compare
|
|
||
| ramp_data_slice.set_arrays(data, err, groupdq, pixeldq) | ||
| ramp_data_slice.set_arrays(data, groupdq, pixeldq, average_dark_current) |
There was a problem hiding this comment.
The gls_fit algorithm is unused and only partially implemented. I noticed in passing that the set_arrays call had incorrect arguments here, so I added the average dark current. Changes in this file should have no impact to any current data processing.
There was a problem hiding this comment.
Sounds good. The gls_fit algorithm should be superseded by the likelihood-based one--it should be the identical calculation, but with orders of magnitude better performance. I think we can consider permanently removing gls_fit.
| @@ -278,6 +278,8 @@ def calc_bias(self, countrates, sig, cvec, da=1e-7): | |||
| Bias of the best-fit count rate from using cvec plus the observed | |||
| resultants to estimate the covariance matrix. | |||
| """ | |||
| raise NotImplementedError('Bias calculations are not implemented.') | |||
There was a problem hiding this comment.
I noticed in passing that this method currently has a code error: it calls a fit_ramps function which is not defined in this module. I added the NotImplementedError to make it clear that it is not working, but this method is not called anywhere. We should decide if it should be removed or fixed in a separate PR.
There was a problem hiding this comment.
Sounds good. I think it can be removed. The calculation is in my repo fit-ramp, but I do not think it is necessary to include in the jwst pipeline.
|
We have a JWST approval from Ken; I'm going to hold off reviewing until someone from Roman gives the go-ahead, just make sure they're aware of the changes we're making. |
|
The romancal regression tests pass on this branch, |
Resolves JP-3827
This PR removes references to the unused error array from the 4D ramp models. In the jwst pipeline, error values are not meaningful until after ramp fitting.
I also did a little minor cleaning for trivial things I noticed while making this change. I'll note any significant differences.
Requires:
spacetelescope/stdatamodels#384
spacetelescope/jwst#9109
Tasks
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)"git+https://github.com/<fork>/stcal@<branch>")jwstregression testromancalregression testnews fragment change types...
changes/<PR#>.apichange.rst: change to public APIchanges/<PR#>.bugfix.rst: fixes an issuechanges/<PR#>.general.rst: infrastructure or miscellaneous change