8000 JP-1729: Assign int_num to extensions in extract_1d by tapastro · Pull Request #6369 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

JP-1729: Assign int_num to extensions in extract_1d #6369

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 7 commits into from
Oct 8, 2021
Merged

JP-1729: Assign int_num to extensions in extract_1d #6369

merged 7 commits into from
Oct 8, 2021

Conversation

tapastro
Copy link
Contributor
@tapastro tapastro commented Sep 29, 2021 •
edited
Loading

Closes #5360

Resolves JP-1729

Description

This PR utilizes the dormant 'INT_NUM' extension keyword in the MultiSpecModel datamodel to track the integration number of each extraction.

Note (to self?) : One regtest will need okify-ing, test_nirspec_brightobj_spec2 -> x1dints.

Checklist

  • Tests
  • Documentation
  • Change log
  • Milestone
  • Label(s)

@tapastro tapastro added this to the Build 7.9 milestone Sep 29, 2021
@codecov
Copy link
codecov bot commented Sep 29, 2021 •
edited
Loading

Codecov Report

Merging #6369 (0fe9794) into master (5d2613b) will decrease coverage by 0.27%.
The diff coverage is 75.00%.

❗ Current head 0fe9794 differs from pull request most recent head f450657. Consider uploading reports for the commit f450657 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6369      +/-   ##
==========================================
- Coverage   77.60%   77.32%   -0.28%     
==========================================
  Files         408      408              
  Lines       34881    35076     +195     
==========================================
+ Hits        27068    27124      +56     
- Misses       7813     7952     +139
Flag Coverage Δ *Carryforward flag
nightly 77.61% <85.71%> (ø) Carriedforward from 365c0e7
unit 56.39% <0.00%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
jwst/extract_1d/extract.py 70.96% <75.00%> (-6.51%) ⬇️
jwst/saturation/saturation.py 97.89% <0.00%> (-2.11%) ⬇️
jwst/saturation/saturation_step.py 82.85% <0.00%> (+6.66%) ⬆️

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 5d2613b...f450657. Read the comment docs.

stscieisenhamer
stscieisenhamer approved these changes Sep 30, 2021
View reviewed changes
Copy link
Collaborator
@stscieisenhamer stscieisenhamer left a comment

Choose a reason for hiding this comment

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

Wow, now that was straight-forward.

hbushouse
hbushouse approved these changes Sep 30, 2021
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.

This obviously fixes the problem, but would it be more appropriate to just call populate_time_keywords for this instrument mode? Of course you'd have to make sure there are hooks in that function to handle the case where INT_TIMES doesn't exist (e.g. still go ahead and populate INT_NUM without the times).

@tapastro
Copy link
Contributor Author
tapastro commented Sep 30, 2021 •
edited
Loading

This obviously fixes the problem, but would it be more appropriate to just call populate_time_keywords for this instrument mode? Of course you'd have to make sure there are hooks in that function to handle the case where INT_TIMES doesn't exist (e.g. still go ahead and populate INT_NUM without the times).

Yes, the issue posed by this ticket at its core is the lack of INT_NUM assignment for a dataset with an empty INT_TIMES table. The pipeline does run populate_time_keywords, but it doesn't populate without a complete INT_TIMES table present in the input. An argument could be made that INT_NUM isn't really a "time keyword", and any extraction input with multiple integrations will get covered by the new code. However the old code will work for any code with a proper INT_TIMES table. I could see many avenues here, including a (likely over-engineered for the result) change to populate_time_keywords that would fill INT_NUM even with an empty INT_TIMES table. I'm not sure what the best course of action is.

@hbushouse
Copy link
Collaborator

It may also end up being confusing for future maintenance to have the INT_NUM keyword populated in two different places in the code, depending on whether the INT_TIMES table has data. So even though it may be more work right now, I think that it'd be better in the long run if the populate_time_keywords function were updated to always populate INT_NUM, even when the INT_TIMES data is not available. At least that way it's all happening in only one place (despite the misnomer of INT_NUM not being a time-related keyword).

@tapastro
Copy link
Contributor Author
tapastro commented Oct 6, 2021

The forced assignment of INT_NUM even with an empty INT_TIMES table is now in populate_time_keywords - requesting another pass over the code before I merge.

@tapastro tapastro merged commit 93ea912 into spacetelescope:master Oct 8, 2021
@stscijgbot-jp stscijgbot-jp mentioned this pull request Oct 8, 2021
Closed
@tapastro tapastro mentioned this pull request Oct 11, 2021
Merged
5 tasks
loicalbert pushed a commit to talensgj/jwst that referenced this pull request Nov 5, 2021
@tapastro @loicalbert
JP-1729: Assign int_num to extensions in extract_1d (spacetelescope#6369
eb511ff 
)

* assign int_num to integrations in extract_1d loop

* [skip ci] changelog

* [skip ci] comment added

* push change into populate_time_keywords

* flake8

* [skip ci] comment update

* [skip ci] changelog update
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 Awaiting requested review from hbushouse

Assignees

@tapastro tapastro

Labels
extract_1d
Projects
None yet
Milestone
Build 7.9
Development

Successfully merging this pull request may close these issues.

labeling of NIRSpec BOTS integrations
3 participants
@tapastro @hbushouse @stscieisenhamer
0