Skip to content
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

RCAL-776: Change image units. #1128

Merged
merged 10 commits into from
Mar 12, 2024

Conversation

PaulHuwe
Copy link
Collaborator

@PaulHuwe PaulHuwe commented Mar 6, 2024

Resolves RCAL-776

Closes #1114 #1123

This PR changed image units in code and tests. Added gain reduction.

NOTE: no regression test link, because these changes require update to regression test files.

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR

@PaulHuwe PaulHuwe requested a review from schlafly March 6, 2024 00:28
@PaulHuwe PaulHuwe requested a review from a team as a code owner March 6, 2024 00:28
@github-actions github-actions bot added ramp_fitting testing dependencies Pull requests that update a dependency file flatfield labels Mar 6, 2024
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.74%. Comparing base (bd465ec) to head (18c2f03).
Report is 1 commits behind head on main.

❗ Current head 18c2f03 differs from pull request most recent head 0b394f9. Consider uploading reports for the commit 0b394f9 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1128      +/-   ##
==========================================
- Coverage   76.72%   75.74%   -0.99%     
==========================================
  Files         105      105              
  Lines        7054     7045       -9     
==========================================
- Hits         5412     5336      -76     
- Misses       1642     1709      +67     
Flag Coverage Δ *Carryforward flag
nightly 62.70% <ø> (-0.04%) ⬇️ Carriedforward from 6c8a97f

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

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

Copy link
Collaborator

@ddavis-stsci ddavis-stsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should wait until the rad and roman_datamodels changes are in. We don't really want the pyproject to point to your local branches.
The code changes look fine.

@PaulHuwe
Copy link
Collaborator Author

PaulHuwe commented Mar 6, 2024

This should wait until the rad and roman_datamodels changes are in. We don't really want the pyproject to point to your local branches. The code changes look fine.

Yeah, they are temporary just for tests to pass. I have updated the Changelog.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 6, 2024
out_model.err /= gain_model.data[4:-4, 4:-4].value
out_model.var_poisson /= gain_model.data[4:-4, 4:-4].value ** 2
out_model.var_rnoise /= gain_model.data[4:-4, 4:-4].value ** 2

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case isn't the important one since we don't use this mode, but instead of adding this code, I think it would be better to delete lines 118-122 and 128-140.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was afraid to remove those in case it affected calculations being made.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the earlier lines do the exact opposite of what you're doing here? We plan to get rid of the support for the old ramp fitting mode eventually anyway, but I don't see why we should multiply in the gain and then divide it back out without doing any work in the meantime? Do you see something I'm missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

image_model.err /= gain[4:-4, 4:-4]
image_model.var_poisson /= gain[4:-4, 4:-4] ** 2
image_model.var_rnoise /= gain[4:-4, 4:-4] ** 2

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Staring at the schema, there is also var_flat, which we don't actually populate or do anything with. I guess that's a separate ticket.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually fine and I was somehow missing the flat field uncertainty step computation later on. Disregard this, sorry!

Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks.

Copy link
Collaborator

@ddavis-stsci ddavis-stsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nden nden mentioned this pull request Mar 12, 2024
6 tasks
@PaulHuwe PaulHuwe merged commit 22293cc into spacetelescope:main Mar 12, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation flatfield ramp_fitting testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report L2 data in DN/s instead of e/s
3 participants