8000 Refactor photom to also update multislit models' bunit keys by jwhite3 · Pull Request #4958 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Refactor photom to also update multislit models' bunit keys #4958

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
Merged
merged 2 commits into from
May 18, 2020
Merged

Refactor photom to also update multislit models' bunit keys #4958

merged 2 commits into from
May 18, 2020

Conversation

jwhite3
Copy link
@jwhite3 jwhite3 commented May 18, 2020

This PR resolves JP-1434.

Fixes a bug where input MultiSlitModel bunit keys were not being updated with new flux units which resulted in incorrect units in the headers of the individual slit products due to header blending down the line

@jwhite3 jwhite3 added the photom label May 18, 2020
hbushouse
hbushouse approved these changes May 18, 2020
View reviewed changes
Copy link
Collaborator
@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

So it was just an indentation issue such that the desired if-block didn't execute in all circumstances? Classic!

Works for me.

@hbushouse hbushouse added this to the Build 7.5 milestone May 18, 2020
@jwhite3
Copy link
Author
jwhite3 commented May 18, 2020

@hbushouse Well sort of, but thankfully the fix seems to be that easy

I think it was a conscious decision for some reason not to include bunit keyword updates in the MultiSlitModel and only apply them in the contained SlitModels. My guess is that this used to not matter since resample_spec didn't blend the headers of the individual slit data, but after we enabled header blending, bunit was getting "updated" back to the original units of "DN/s"

@codecov
Copy link
codecov bot commented May 18, 2020 •
edited
Loading

Codecov Report

Merging #4958 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4958   +/-   ##
=======================================
  Coverage   52.46%   52.46%           
=======================================
  Files         403      403           
  Lines       35731    35723    -8     
  Branches     5542     5543    +1     
=======================================
- Hits        18745    18742    -3     
+ Misses      15848    15843    -5     
  Partials     1138     1138
Flag Coverage Δ
#unit 52.46% <100.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
jwst/photom/photom.py 57.99% <100.00%> (ø)
jwst/datamodels/fits_support.py 81.65% <0.00%> (-0.54%) ⬇️
jwst/datamodels/make_header.py 84.15% <0.00%> (-0.05%) ⬇️
jwst/timeconversion/time_conversion.py 3.24% <0.00%> (+0.02%) ⬆️
jwst/fits_generator/template.py 17.25% <0.00%> (+0.26%) ⬆️
jwst/associations/lib/process_list.py 72.83% <0.00%> (+0.55%) ⬆️
jwst/associations/asn_edit.py 61.79% <0.00%> (+0.68%) ⬆️

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 1f85ada...d33ceea. Read the comment docs.

@hbushouse
Copy link
Collaborator

Needs a change log entry.

@hbushouse hbushouse merged commit 0f51a32 into spacetelescope:master May 18, 2020
@stscijgbot-jp stscijgbot-jp mentioned this pull request May 19, 2020
Closed
stscieisenhamer pushed a commit to stscieisenhamer/jwst that referenced this pull request Jun 5, 2020
@stscieisenhamer
Refactor photom to also update multislit models' bunit keys (spacetel…
774e422 
…escope#4958)

* Refactor to also update multislit models' bunit keys

* Add change note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stscieisenhamer stscieisenhamer stscieisenhamer approved these changes

@hbushouse hbushouse hbushouse approved these changes

Assignees
No one assigned
Labels
photom
Projects
None yet
Milestone
Build 7.5
Development

Successfully merging this pull request may close these issues.
3 participants
@jwhite3 @hbushouse @stscieisenhamer
0