Rcal 406 science array quantities #613

PaulHuwe · 2022-12-14T12:27:01Z

Resolves RCAL-406

Closes #

This PR adds support for Quantities for data arrays.

Checklist

added entry in CHANGES.rst under the corresponding subsection
updated relevant tests
updated relevant documentation
updated relevant milestone(s)
added relevant label(s)

…o ArrayQuanitites

codecov · 2022-12-14T12:29:42Z

Codecov Report

Base: 81.70% // Head: 75.99% // Decreases project coverage by -5.71% ⚠️

Coverage data is based on head (cd10324) compared to base (12ddbe8).
Patch coverage: 82.75% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #613      +/-   ##
==========================================
- Coverage   81.70%   75.99%   -5.72%     
==========================================
  Files          39       39              
  Lines        1170     1283     +113     
==========================================
+ Hits          956      975      +19     
- Misses        214      308      +94

Flag	Coverage Δ		*Carryforward flag
nightly	`81.19% <100.00%> (-0.52%)`	⬇️	Carriedforward from f9b44bc
unit	`46.63% <32.00%> (+0.32%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
romancal/linearity/linearity_step.py	`64.28% <66.66%> (-17.30%)`	⬇️
romancal/flatfield/flat_field.py	`64.28% <72.72%> (-24.36%)`	⬇️
romancal/dark_current/dark_current_step.py	`64.89% <80.00%> (-24.94%)`	⬇️
romancal/jump/jump_step.py	`97.36% <100.00%> (ø)`
romancal/ramp_fitting/ramp_fit_step.py	`72.47% <100.00%> (-24.86%)`	⬇️
romancal/saturation/saturation.py	`100.00% <100.00%> (ø)`
... and 2 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

ddavis-stsci

LGTM

schlafly

This looks good to me, with some comments.

My biggest concern is that the value of having units is that they prevent you from doing things that don't make sense, e.g., 1u.DN + 1u.electron triggers an exception. For the most part this PR avoids this feature by working primarily with quantity.value, and just treating things as ordinary numpy arrays. I see that as a last resort and think we should be trying to avoid ".value" and Quantity(...) when possible.

Of course, it's not always possible---stcal will want just the plan numpy arrays, and we'll need to redecorate them with units when we get them back from stcal. But I noted a number of cases in the code where I think we could be treating the units more naturally.

schlafly · 2022-12-14T19:05:54Z

romancal/flatfield/flat_field.py

@@ -109,21 +111,21 @@ def apply_flat_field(science, flat):
    flat_data[np.where(flat_bad)] = 1.0
    # Now let's apply the correction to science data and error arrays.  Rely
    # on array broadcasting to handle the cubes
-    science.data /= flat_data
+    science.data = u.Quantity((science.data.value / flat_data), ru.electron / u.s, dtype=science.data.dtype)


What units does science.data start with We already forced it to have e/s in the schema? flat_data is probably dimensionless, so I would have guessed that science.data /= flat_data would just work? Am I missing something?

schlafly · 2022-12-14T19:07:35Z

romancal/flatfield/flat_field.py

+                                  ru.electron ** 2 / u.s ** 2, dtype=science.data.value.dtype)
+
+    err_sqrt = np.sqrt(science.var_poisson.value + science.var_rnoise.value + science.var_flat)
+    science.err = u.Quantity(err_sqrt, ru.electron / u.s, dtype=err_sqrt.dtype)


Likewise, I think my expectation throughout here is that the old code would be fine
e.g. science.var_poisson /= flat_data_squared should preserve the units?
Same for science.var_rnoise, science.var_flat, and only the exception should require explicit quantities?

schlafly · 2022-12-14T19:10:52Z

romancal/jump/tests/test_jump_step.py

@@ -142,7 +144,7 @@ def test_one_CR(generate_wfi_reffiles, max_cores, setup_inputs):
                          deltatime=grouptime)

    for i in range(ngroups):
-        model1.data[i, :, :] = deltaDN * i
+        model1.data.value[i, :, :] = deltaDN * i


I would have said it's more idiomatic to change deltaDN -> deltaDN * u.DN that it is to have all the data.values.

schlafly · 2022-12-14T19:11:20Z

romancal/jump/tests/test_jump_step.py

-        model1.data[CR_group:, CR_y_locs[i], CR_x_locs[i]] = \
-            model1.data[CR_group:, CR_y_locs[i], CR_x_locs[i]] + 500.
+        model1.data.value[CR_group:, CR_y_locs[i], CR_x_locs[i]] = \
+            model1.data.value[CR_group:, CR_y_locs[i], CR_x_locs[i]] + 500.


Likewise my default here would have been 500 -> 500 * u.DN rather than working directly with the values.

This reverts commit 9336348.

* Revert "Rcal 406 science array quantities (#613)" This reverts commit 9336348. * Removed romancal from the requirements for romancal. * Added RAD versions.

PaulHuwe added 13 commits October 4, 2022 12:50

Rough test of array quanitties.

d4349c8

Changed Quantity object instantiation in order to preserve datatype.

0af0a45

Saving for rebase.

89ba28b

Merge branch 'main' of https://github.com/spacetelescope/romancal int…

75d1d95

…o ArrayQuanitites

Updates unit tests.

f4a663a

Code clean up.

d4dfd2d

Updated Changelog.

3ede27a

Corrected some array dtypes for quantities.

c47288c

Updated tests for units.

a6de70c

Temporarilly pointing to PR of RDM.

c8ae852

Code cleanup.

0fea09b

Rebase.

2c59414

Temporarilly redirected RDM link.

f6c8140

github-actions bot added Dark Current dependencies Pull requests that update a dependency file dq_init flatfield jump linearity ramp_fitting Saturation testing labels Dec 14, 2022

Updated Changelog.

f9b44bc

PaulHuwe self-assigned this Dec 14, 2022

PaulHuwe marked this pull request as ready for review December 14, 2022 12:49

PaulHuwe requested review from nden, ddavis-stsci and schlafly December 14, 2022 12:49

ddavis-stsci approved these changes Dec 14, 2022

View reviewed changes

schlafly reviewed Dec 14, 2022

View reviewed changes

Updated asdf dependency to match RDM.

cd10324

PaulHuwe merged commit 9336348 into spacetelescope:main Dec 15, 2022

PaulHuwe added a commit that referenced this pull request Dec 15, 2022

Revert "Rcal 406 science array quantities (#613)"

345174a

This reverts commit 9336348.

PaulHuwe mentioned this pull request Dec 15, 2022

Revert "Rcal 406 science array quantities" #614

Merged

PaulHuwe added a commit that referenced this pull request Dec 15, 2022

Revert "Rcal 406 science array quantities" (#614)

fb7f84c

* Revert "Rcal 406 science array quantities (#613)" This reverts commit 9336348. * Removed romancal from the requirements for romancal. * Added RAD versions.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rcal 406 science array quantities #613

Rcal 406 science array quantities #613

PaulHuwe commented Dec 14, 2022

codecov bot commented Dec 14, 2022 •

edited

Loading

ddavis-stsci left a comment

schlafly left a comment

schlafly Dec 14, 2022

schlafly Dec 14, 2022

schlafly Dec 14, 2022

schlafly Dec 14, 2022

Rcal 406 science array quantities #613

Rcal 406 science array quantities #613

Conversation

PaulHuwe commented Dec 14, 2022

codecov bot commented Dec 14, 2022 • edited Loading

Codecov Report

ddavis-stsci left a comment

Choose a reason for hiding this comment

schlafly left a comment

Choose a reason for hiding this comment

schlafly Dec 14, 2022

Choose a reason for hiding this comment

schlafly Dec 14, 2022

Choose a reason for hiding this comment

schlafly Dec 14, 2022

Choose a reason for hiding this comment

schlafly Dec 14, 2022

Choose a reason for hiding this comment

codecov bot commented Dec 14, 2022 •

edited

Loading