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.

Sign up for GitHub

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

Jump to bottom

RCAL-322: Tests for SOC-588 (Absolute photometric calibration) #479

Merged
merged 21 commits into from
Apr 29, 2022
Merged

RCAL-322: Tests for SOC-588 (Absolute photometric calibration) #479

merged 21 commits into from
Apr 29, 2022

Conversation

PaulHuwe
Copy link
Collaborator

GitHub issue, Closes #474

Resolves RCAL-322

Description

This PR adds SOC test for absolute photometric calibration.
Also, tweaked logging in photom step.
Updated rad version compatibility in setup.cfg.

Checklist

  • Tests

  • Documentation

  • Change log

  • Milestone

  • Label(s)

PaulHuwe added 17 commits March 30, 2022 21:09
@PaulHuwe
Added photom correction step and unit tests.
1e949bb
@PaulHuwe
Updated Changelog.
56a132d
@PaulHuwe
Corrected flake8 issues.
3be9911
@PaulHuwe
Added CRDS check and skip for a test.
4859c8d
@PaulHuwe
Updated and streamlined code as per PR comments.
3e9798c
@PaulHuwe
Fixed flake8 issues.
fb18b96
@PaulHuwe
Pointed RDM to dev version.
d2c5f55
@PaulHuwe
Addressed additional PR comments.
141f7dc
@PaulHuwe
Addressed PR comments.
9ac3389
@PaulHuwe
Updated to main.
fc4853d
@PaulHuwe
Removed extranneous line.
b6bea7a
@PaulHuwe
Saving work.
a94f1e4
@PaulHuwe
Addressed PR comments.
957f5eb
@PaulHuwe
Merge branch 'RCAL-112_Photom' of https://github.com/PaulHuwe/romancal …
9b66346 
…into RCAL-323_SOC588Tests
@PaulHuwe
Merge branch 'main' of https://github.com/spacetelescope/romancal int…
5b506a1 
…o RCAL-323_SOC588Tests
@PaulHuwe
Added SOC test for absolute photometric calibration. Tweak logging in…
b11d01d 
… photom step.
@PaulHuwe
Updated Changelog.
6ea7158
@stscijgbot-rstdms stscijgbot-rstdms mentioned this pull request Apr 20, 2022
Closed
ddavis-stsci
ddavis-stsci approved these changes Apr 20, 2022
View reviewed changes
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.

How did you pick your tolerances? I'm surprised that the tolerance for the megaJy and micro Jy uncertainties are the same for an absolute tolerance.
Overall it looks like it tests what we need.

@PaulHuwe
Copy link
Collaborator Author

Files are in artifactory.

@PaulHuwe
Copy link
Collaborator Author

I chose the tolerances relative to the values calculated from the matching photom file. I agree that ideally the tolerances for the uncertainties would be different than for the absolute values, but given that the uncertainties are 0 in the CRDS selected photom file, there wasn't a good base to set them off of - hence being the same.

@codecov
Copy link

codecov bot commented Apr 26, 2022 •
edited
Loading

Codecov Report

Merging #479 (b36ac6f) into main (c0cf0f5) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #479   +/-   ##
=======================================
  Coverage   77.69%   77.69%           
=======================================
  Files          39       39           
  Lines        1031     1031           
=======================================
  Hits          801      801           
  Misses        230      230
Flag Coverage Δ *Carryforward flag
nightly 78.29% <100.00%> (ø) Carriedforward from c0cf0f5

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

Impacted Files Coverage Δ
romancal/photom/photom.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0cf0f5...b36ac6f. Read the comment docs.

nden
nden reviewed Apr 26, 2022
View reviewed changes
Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

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

Tests pass now.

setup.cfg Outdated
@@ -30,7 +30,7 @@ install_requires =
pyparsing>=2.2
requests>=2.22
roman_datamodels>=0.11.0
romanad==0.10.0
rad>=0.11.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove rad altogether since it comes with romand_datamodels. Is romancal using anything directly from rad or even importing it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rad can be removed. It was there when we needed an earlier version of rad to pass the tests and is no longer needed.

@PaulHuwe PaulHuwe merged commit da385fb into spacetelescope:main Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddavis-stsci ddavis-stsci ddavis-stsci approved these changes

@nden nden nden approved these changes

@cshanahan1 cshanahan1 Awaiting requested review from cshanahan1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tests for SOC-588 (Absolute photometric calibration)
3 participants
@PaulHuwe @nden @ddavis-stsci