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

NIRISS WFSS set SRC_TYPE in extract_2d JP-1917 #5788

Merged
merged 7 commits into from
Mar 9, 2021

Conversation

jemorrison
Copy link
Collaborator

@jemorrison jemorrison commented Feb 25, 2021

This pr concerns JP-1917.
The GrismObject defined in transforms/model.py was updated to include 'is_star'
assign_wcs/util.py also had to be updated to pass this information along.

The code that sets SRC_TYPE for each slit in a NIRISS WFSS is in extract_2d/grism.py

A question I have is in the srctype step there is code that sets input_model.meta.target.source_type = src_type.
The log message to screen for running NIRISS WFSS in spec2 using a source catalog reports:
2021-02-25 15:56:15,579 - stpipe.Spec2Pipeline.srctype - WARNING - EXP_TYPE NIS_WFSS not applicable to this operation
2021-02-25 15:56:15,580 - stpipe.Spec2Pipeline.srctype - WARNING - Setting SRCTYPE = UNKNOWN
But I am not sure what input_model.meta.target.source_type is For NIRISS WFSS. We need to set the SRCTYPE for each slit not the input_model.meta.target.source_type.
Running this branch on NIRISS WFSS data results in the x1d files with SRCTYPE = 'POINT' for the is_star = True values in the source catalog.

Fixes #5731 / JP-1917

@jemorrison
Copy link
Collaborator Author

@hbushouse @sosey I don't think these changes will affect other GRISM type data. The GrismObject may contain the is_star information but it is only used in extract_2d/grism.py so I don't think these changes will break any thing else.

@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #5788 (7975f28) into master (72327bc) will increase coverage by 0.15%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5788      +/-   ##
==========================================
+ Coverage   75.81%   75.97%   +0.15%     
==========================================
  Files         406      406              
  Lines       36608    36882     +274     
==========================================
+ Hits        27756    28021     +265     
- Misses       8852     8861       +9     
Flag Coverage Δ *Carryforward flag
nightly 75.66% <100.00%> (ø) Carriedforward from 68f52b7
unit 54.49% <33.33%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
jwst/srctype/srctype.py 92.59% <83.33%> (+2.05%) ⬆️
jwst/assign_wcs/util.py 79.14% <100.00%> (-0.86%) ⬇️
jwst/extract_2d/grisms.py 87.55% <100.00%> (ø)
jwst/transforms/models.py 93.32% <100.00%> (+1.56%) ⬆️

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 72327bc...7975f28. Read the comment docs.

@hbushouse
Copy link
Collaborator

How about the idea of adding is_star to the SlitModel schema, but only as a model attribute and not tied to a FITS keyword (i.e. don't give it a fits_keyword entry in the schema definition), and set it for each WFSS slit instance created in extract_2d/grisms.py (it would not get set for any other exposure types that use SlitModel), such that we can then add a special WFSS processing block in the srctype step to simply set the SRCTYPE keyword for each slit instance based on the value of is_star.

I know it's a bit hokey to define a new model attribute just to carry one piece of info to the very next step where it'll get replaced or superseded by another model attribute, but at least that way we maintain consistency in terms of having the srctype step actually set the SRCTYPE keyword value for all exposure types (rather than having it happen in extract_2d for WFSS exposures).

@jemorrison
Copy link
Collaborator Author

Just to be clear we would leave the updates to the GrismObject into include is_star (since that information is read from the source catalog). But add is_star to slit_model and set it in extract_2d/grism.py Then we can set the SRCTYPE in the scrtype step where it really should be.
That is cleaner than setting SRCTYPE in extract_2d

@hbushouse
Copy link
Collaborator

Just to be clear we would leave the updates to the GrismObject into include is_star (since that information is read from the source catalog). But add is_star to slit_model and set it in extract_2d/grism.py Then we can set the SRCTYPE in the scrtype step where it really should be.
That is cleaner than setting SRCTYPE in extract_2d

Right. Include is_star in the GrismObjects when they're created, then have extract_2d store the value of is_star into the slit.meta when it creates each slit instance, then finally have srctype read is_star from the slit meta and set SRCTYPE. The operation in srctype would then look very much like what it does for NIRSpec MOS data, where it loops over all the slits contained in the input MultiSlitModel and sets SRCTYPE for each slit based on the value of stellarity. In this case, for WFSS data, it would use is_star instead of stellarity.

@jemorrison jemorrison force-pushed the JP-1917_NIRISS_WFSS branch from 71f4976 to 27a4aff Compare March 1, 2021 14:55
@@ -476,7 +476,10 @@ def extract_grism_objects(input_model,
# The overall subarray offset is recorded in model.meta.subarray.
# nslit = obj.sid - 1 # catalog id starts at zero
new_slit.name = "{0}".format(obj.sid)
new_slit.source_type = 'UNKNOWN'
if obj.is_star:
new_slit.is_star = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think any logic is needed here to make a choice of what to set, but instead simply set new_slit.is_star = obj.is_star, in order to carry along the value of is_star to the srctype step, where it will set the appropriate value of source_type.

Copy link
Collaborator Author

@jemorrison jemorrison Mar 5, 2021

Choose a reason for hiding this comment

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

yes I meant to remove the settling of source type. Some how my push did not include this update. I did it on a different branch by mistake :(

@@ -156,6 +156,14 @@ def set_source_type(input_model):
log.info(f'Input is a TSO exposure; setting default SRCTYPE = {src_type}')
input_model.meta.target.source_type = src_type

# FOR NIR_WFSS check slit values of is_star to set SRCTYPE
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be done for all WFSS modes, so that includes NRC_WFSS and NIS_WFSS. This means that some of the change log entries above need to be updated to indicate that these changes apply to WFSS in general, not just NIS_WFSS.

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 am not all the familiar with WFSS mode. I did a search on WFSS in the pipeline and frequently get these modes:
WFSS_EXPTYPES = ['NIS_WFSS', 'NRC_WFSS', 'NRC_GRISM', 'NRC_TSGRISM']
NRC_TSGRISM is a mode that is already handled in the srctype code. But I could not find NRC_GRISM
Do I need to add it to this too ?

CHANGES.rst Outdated
extract_2d
----------

- For NIS_WFSS removed setting sorce_type to UNKNOW, added setting is_star to slitmeta [#5788]
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be for all WFSS now, not just NIS_WFSS

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.

Looks good, except for one minor wording issue in change log.

@jemorrison jemorrison force-pushed the JP-1917_NIRISS_WFSS branch from b3d9e57 to 68f52b7 Compare March 8, 2021 14:42
@hbushouse hbushouse merged commit 0744f5b into spacetelescope:master Mar 9, 2021
@jemorrison jemorrison deleted the JP-1917_NIRISS_WFSS branch April 12, 2021 13:44
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.

SRCTYPE header keyword not populated in x1d files (NIRISS)
2 participants