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

JP-1791: Initial commit of WFSS contam correction step and INS grism library modules #5508

Merged
merged 57 commits into from
May 11, 2021

Conversation

hbushouse
Copy link
Collaborator

@hbushouse hbushouse commented Dec 3, 2020

These are the code modules from Nor Pirzkal's various grism repos that we need in order to support the WFSS contamination corrections. These modules do the work of creating simulated spectra for sources identified in direct images. These will all need a lot of clean-up and rearranging to get them to work from within the jwst pipeline environment. Just getting them into the repo to start with. Some of them may end up going away eventually. The functions/methods in these modules will be called by a new calwebb_spec2 step that will do decontamination of WFSS spectra.

Resolves JP-1791
Resolves #5503
Resolves JP-1924
Resolves #5750

@sosey
Copy link
Member

sosey commented Dec 3, 2020

for background, the essence of a lot of these are already implemented in jwst as part of the 2b spec processing and reference files

@hbushouse
Copy link
Collaborator Author

for background, the essence of a lot of these are already implemented in jwst as part of the 2b spec processing and reference files

Good to know. I assumed that a lot or all of what these routines are doing with grism Config (calibration) info is already in the WCS work that you did for grisms and anything dealing with that kind of stuff will need to be migrated to make use of the WCS objects.

@sosey
Copy link
Member

sosey commented Dec 3, 2020

yah, grismconf and the parts of poly we ended up using are in transforms/models

@hbushouse hbushouse marked this pull request as draft December 8, 2020 13:52
@hbushouse hbushouse marked this pull request as draft December 8, 2020 13:52
@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #5508 (04385a1) into master (6609329) will decrease coverage by 0.64%.
The diff coverage is 15.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5508      +/-   ##
==========================================
- Coverage   77.81%   77.17%   -0.65%     
==========================================
  Files         399      405       +6     
  Lines       35354    35740     +386     
==========================================
+ Hits        27512    27583      +71     
- Misses       7842     8157     +315     
Flag Coverage Δ *Carryforward flag
nightly 77.82% <50.00%> (ø) Carriedforward from 6609329
unit 56.49% <15.14%> (-0.38%) ⬇️

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

Impacted Files Coverage Δ
jwst/lib/suffix.py 98.50% <ø> (+2.50%) ⬆️
jwst/pipeline/calwebb_spec2.py 76.74% <0.00%> (-19.32%) ⬇️
jwst/stpipe/integration.py 100.00% <ø> (ø)
jwst/wfss_contam/observations.py 8.88% <8.88%> (ø)
jwst/wfss_contam/disperse.py 11.90% <11.90%> (ø)
jwst/wfss_contam/wfss_contam.py 21.42% <21.42%> (ø)
jwst/wfss_contam/sens1d.py 25.92% <25.92%> (ø)
jwst/wfss_contam/wfss_contam_step.py 47.05% <47.05%> (ø)
jwst/source_catalog/source_catalog_step.py 89.06% <100.00%> (-1.27%) ⬇️
jwst/step.py 100.00% <100.00%> (ø)
... and 9 more

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 6609329...04385a1. Read the comment docs.

@hbushouse
Copy link
Collaborator Author

@jdavies-st thanks for the comments. There's a lot of this that will be reorganized, if not completely rewritten, yet and some of it (e.g. grismconf and poly) will go away once I'm done migrating the rest of the code over to getting everything we need from our WCS objects and ref files, rather than the aXe-style config files. I'll try to incorporate your suggestions as I go.

But hey, at least I finally got the Travis build to succeed!

@nden nden modified the milestones: Build 7.7, Build 7.7.x Jan 11, 2021
@hbushouse hbushouse marked this pull request as ready for review May 7, 2021 19:25
@hbushouse
Copy link
Collaborator Author

Latest large set of commits contains LOTS of cleanup, removal of unused functions, complete code commentary, and new functions for handling flux calibration (actually inverse flux calibration) where needed. Reopening for review and hope to merge soon. This version has all of the basic mechanics in place. Still needs a couple of enhancements to handle multiple spectral orders and get the scaling of simulated spectra correct. Those changes will follow in a later PR. Just want to get the basics merged in for now.

@hbushouse
Copy link
Collaborator Author

Complete user docs for the new step and unit tests will also follow in subsequent PRs.

@hbushouse hbushouse changed the title JP-1791: Initial commit of INS grism library modules JP-1791: Initial commit of WFSS contam correction step and INS grism library modules May 7, 2021
@hbushouse hbushouse dismissed nden’s stale review May 10, 2021 14:00

Original C module has been replaced, thus there's no longer an issue with the license.

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.

LGTM. I left a couple minor comments inline.

@hbushouse hbushouse merged commit ed5a6ea into spacetelescope:master May 11, 2021
@hbushouse hbushouse deleted the wfss_contam branch May 11, 2021 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new step prototype in calspec2 for WFSS contam correction Move WFSS contamination code into pipeline
5 participants