JP-3463: Utilize average_dark_current in variance calculations

[tapastro]

Resolves JP-3463

Duplicates work in #236

This PR passes the newly added RampModel.average_dark_current to the RampData class, then uses it during poisson variance calculations.

Checklist

• added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
• updated relevant tests
• updated relevant documentation
• updated relevant milestone(s)
• added relevant label(s)

[codecov]

Codecov Report

Attention: Patch coverage is 76.66667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 85.87%. Comparing base (41259f3) to head (bb61955).

Files	Patch %	Lines
src/stcal/ramp_fitting/ramp_fit.py	0.00%	5 Missing ⚠️
src/stcal/ramp_fitting/ols_fit.py	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #243      +/-   ##
==========================================
- Coverage   85.90%   85.87%   -0.03%     
==========================================
  Files          35       35              
  Lines        6557     6572      +15     
==========================================
+ Hits         5633     5644      +11     
- Misses        924      928       +4     

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

[kmacdonald-stsci]

Is avg_dark_currrent an array or a single value? If it's a single value, it should be passed to the RampData class using a different method. I approved this PR in case it is an array, but if it's not an array, please change the method usage to set_meta.

[tapastro]

Looks like I need a concurring review - @hbushouse ?

[hbushouse]

Do any of these changes involve changes to API for functions that are called from jwst/ramp_fitting? That'll determine whether we make the new stcal release 1.6.1 or 1.7.0.

[tapastro]

I think the answer would be yes, it does. The API arguments aren't changing, in that the number and type of objects passed are not changing; however, the RampModel passed between jwst/ramp_fitting <-> stcal/ramp_fitting is expected to have certain attributes that won't be present if the versions are not in sync.

[hbushouse]

LGTM

[hbushouse]

I think the answer would be yes, it does. The API arguments aren't changing, in that the number and type of objects passed are not changing; however, the RampModel passed between jwst/ramp_fitting <-> stcal/ramp_fitting is expected to have certain attributes that won't be present if the versions are not in sync.

OK, that makes sense. Even though function calls aren't changing, the fact that we must have matching versions of code between stcal and jwst does warrant the stcal release being bumped up to 1.7.0. So I think that means you need to update the change log entry (I think it said "1.6.1")?

[hbushouse]

@nden Can you remind us who from Roman are the appropriate folks to test this PR before we merge it?

[schlafly]

Thanks, yes, one of us should run the Roman regtests with this PR; I can look at that tomorrow. I don't expect this to cause issues since we use the other ramp fitting approach, though I could imagine some of the ramp fit class API changes causing issues.

[braingram]

I started the romancal regtests here:
https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/618/

but then noticed that the downstream jwst and romancal jobs are failing in the CI (so I expect the romancal regtests to also fail).

@tapastro can you confirm that the failures in the jwst downstream job are fixed by:
spacetelescope/stdatamodels#265

@schlafly is it expected to see failures like:
E AttributeError: No such attribute (average_dark_current) found in node
in the romancal downstream job for this PR?

[tapastro]

I have a proposed fix for the roman side - I have a question on implementation, though. I didn't realize this code was used by both missions, so I wrote it assuming roman datamodels would not see it. So, I need to supply a dummy array if the input model to the RampData does not have an average_dark_current attribute. Would a Roman datamodel (RampModel?) allow me to assign a new attribute, e.g. roman_model.average_dark_current = np.zeros_like(roman_model.pixeldq)? The first version of the fix initializes a new array instead.

[braingram]

Unfortunately the data models do not allow arbitrary attribute assignment.

>> m.average_dark_current = np.zeros((30, 30), dtype='f4')
AttributeError: No such attribute (average_dark_current) found in node

It is possible to use a key:

>> m['average_dark_current'] = np.zeros((30, 30), dtype='f4')

but I don't know if that's recommended.

Most of the failures appear in:
https://github.com/spacetelescope/romancal/blob/main/romancal/ramp_fitting/tests/test_ramp_fit_ols.py
which is testing the ols method that is listed as an option (but not the default) for the RampFitStep:
https://github.com/spacetelescope/romancal/blob/main/romancal/ramp_fitting/ramp_fit_step.py#L29

If there are no plans to use ols perhaps a fix would be to remove that option (and the tests) in romancal.

[braingram]

It looks like all the romancal regtest failures match the ones in the CI so it's likely that getting the CI to go "green" will result in a clean regtest run for romancal.

[braingram]

Nice! This (and your other changes) appear to have appeased romancal. Let me know when you're ready for another regtest run.

[schlafly]

Thank you both! Yes, I think a mode where, if not set, average_dark_current were treated as zero would be the best for us. Yes, we also intend to eventually remove support for the ols mode (since we need to be able to support uneven ramps), but we weren't planning on doing that now.