JP-1431: add parameter extract_height to wfss extraction #5140

nden · 2020-07-10T20:14:17Z

nden · 2020-07-10T20:14:58Z

@hbushouse Is the extraction_height parameter supposed to be a full or half height?

codecov · 2020-07-10T20:43:24Z

Codecov Report

Merging #5140 into master will increase coverage by 0.00%.
The diff coverage is 60.41%.

@@           Coverage Diff           @@
##           master    #5140   +/-   ##
=======================================
  Coverage   52.53%   52.54%           
=======================================
  Files         406      406           
  Lines       36247    36265   +18     
  Branches     5615     5620    +5     
=======================================
+ Hits        19044    19056   +12     
- Misses      16026    16031    +5     
- Partials     1177     1178    +1

Flag	Coverage Δ
#unit	`52.54% <60.41%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
jwst/extract_2d/extract_2d.py	`50.00% <0.00%> (ø)`
jwst/extract_2d/extract_2d_step.py	`100.00% <ø> (ø)`
jwst/lib/catalog_utils.py	`94.73% <ø> (ø)`
jwst/source_catalog/source_catalog.py	`27.22% <0.00%> (ø)`
jwst/assign_wcs/util.py	`51.03% <51.51%> (+0.11%)`	⬆️
jwst/datamodels/wcs_ref_models.py	`50.88% <100.00%> (+0.58%)`	⬆️
jwst/extract_2d/grisms.py	`75.57% <100.00%> (ø)`
jwst/datamodels/source_container.py	`57.14% <0.00%> (-3.73%)`	⬇️

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 65c6882...712cd05. Read the comment docs.

hbushouse · 2020-07-10T21:53:03Z

JP-1431 requests that the extraction parameter be a half-height, such that param=N results in a total height of 2N+1.

jdavies-st

Does this also allow all the grism extractions to have their height set to a specific value, not just SOSS? That could be useful for the WFSS modes as well I suspect.

jdavies-st · 2020-07-11T20:35:24Z

jwst/assign_wcs/util.py

@@ -506,7 +506,8 @@ def get_object_info(catalog_name=None):
 def create_grism_bbox(input_model,
                      reference_files,
                      mmag_extract=99.0,
-                      extract_orders=None):
+                      extract_orders=None,
+                      extract_height=None):


If the keyword is extract_height it would be clearer if it was the full height of the extraction. And this is already what the current keyword is.

jwst/jwst/extract_2d/extract_2d.py

Lines 37 to 40 in ff9ea40

extract_height: int

Cross-dispersion extraction height to use for time series grisms.

This will override the default which for NRC_TSGRISM is a set

size of 64 pixels.

If it is to be the half height, then extraction_half_height would be clearer to users I think.

jdavies-st · 2020-07-11T20:37:18Z

jwst/assign_wcs/util.py

+            _extract_height_direction = 'y'
+        else:
+            _extract_height_direction = 'x'


The DISPAXIS or meta.wcsinfo.dispersion_direction keyword should give the dispersion direction without having to rely on filter name parsing. Or move the filter name parsing to the dispersion direction function

jwst/jwst/lib/dispaxis.py

Lines 6 to 8 in ff9ea40

def get_dispersion_direction(exposure_type, grating="ANY", filter_wh="ANY",

pupil="ANY"):

"""Get the dispersion direction.

hbushouse · 2020-07-12T17:40:12Z

Does this also allow all the grism extractions to have their height set to a specific value, not just SOSS? That could be useful for the WFSS modes as well I suspect.

This PR IS for WFSS mode, not SOSS.

jdavies-st · 2020-07-13T14:39:46Z

Doh! brainfart

Then the extract_height keyword should be consistent with what it is for SOSS, i.e. the full height of the extraction box. Having the same keyword mean 2 different things with two different datasets using the same code is not good.

nden · 2020-07-17T15:18:12Z

@jdavies-st SOSS does not go through extract_2d. The parameter is for TSGRISM mode. But it's a good point. @hbushouse Would it be possible to change the TSGRISM parameter to "half height" as well or does the INS team want is to be a full height. If so we'll have to have two parameters.

hbushouse · 2020-07-17T15:27:08Z

I know the NIRCam TSO folks were specific about the placement and height of the extraction box that they wanted for TSGRISM. If the current height happens to be an odd number, then it too could be expressed as a half-height, like WFSS. But I'm guessing that the default values will be different for TSGRISM and WFSS, so it's not possible to combine into a single parameter, unless we create separate extract_2d parameter files and install them in CRDS. During processing, the appropriate one would be selected based on EXP_TYPE.

nden · 2020-07-17T15:33:25Z

In this case I'll create a new parameter extract_half_hight_grism and say the rest is out of scope for this PR. If there are better ideas, please speak.

hbushouse · 2020-07-17T15:35:52Z

That's fine, but I would use the term "wfss" in the parameter name, instead of the generic "grism", because TSGRISM and WFSS both use a grism. This parameter will be specific to WFSS only. Perhaps "wfss_extract_half_height".

add change log

nden · 2020-07-18T21:26:12Z

I took the opportunity to refactor create_grism_bbox by splitting it into two functions (unfortunately still long), one that performs the computations and one that sets the input parameters. I also added another optional parameter, wavelength_range, to create_grim_bbox which gives more control over setting the extraction parameters.

docs/jwst/extract_2d/main.rst

hbushouse · 2020-07-19T16:04:51Z

docs/jwst/extract_2d/main.rst

+For NIRCam and NIRISS WFSS mode, the ``extract_2d`` step has one optional argument:
+
+  ``--wfss_extract_half_height``
+    int. The cross-dispersion half size, in pixels.


Again, note that this only applies to sources tagged in the source catalog as point-like (based on the catalog entry "is_star" - boolean).

will make that check and point this pout in the documentation

The catalog I have lists is_star as float, not boolean, and all values are NaNs.

<Column name='is_star' dtype='float64' description='Flag indicating whether the source is a star' length=116> nan nan nan nan

Do you know if this column is computed in photutils or source_catalog?

From source_catalog:

@lazyproperty def is_star(self): """ Flag indicating whether the source is a star. 2020.03.02: The JWST Photometry Working Group has not determined the criteria for this flag. """ # TODO: need algorithm for this flag #is_star = np.random.randint(2, size=len(self.id)) #return is_star.astype(bool) return self.null_column

This is where NaNs are returned.

An algorithm was specified a month or two ago, but it hasn't been implemented yet. It will be Boolean when implemented, with a value of True for point sources.

hbushouse · 2020-07-19T16:06:29Z

jwst/assign_wcs/util.py

+        Cross-dispersion extraction half height in pixels, WFSS mode.
+        Overwrites the computed extraction height in ``GrismObject.order_bounding.``
+        If ``None``, it's computed from the segementation map,
+        using the min and max wavelegnth for each of the orders that


typo in "wavelegnth"

hbushouse · 2020-07-19T16:07:52Z

jwst/assign_wcs/util.py

+            message = "If reference files are not supplied, ``wavelength_range`` must be provided."
+            raise TypeError(message)
+    else:
+        # Get the list of extract_orders and lmin, lmax from the ``wavelengthrange11 reference file.


typo "wavelengthrange11"

jwst/assign_wcs/util.py

hbushouse · 2020-07-19T16:12:16Z

jwst/extract_2d/extract_2d_step.py

@@ -16,7 +16,8 @@ class Extract2dStep(Step):
    spec = """
        slit_name = string(default=None)
        extract_orders = int_list(default=None)  # list of orders to extract
-        extract_height =  integer(default=None)  # extraction height in pixels
+        extract_height =  integer(default=None)  # extraction height in pixels, TSGRISM mode


Since we now have 2 extraction height params, maybe we should be explicit in the names for both of them which mode they apply to, such as tsgrism_extract_height.

hbushouse · 2020-07-19T16:13:16Z

jwst/extract_2d/grisms.py

@@ -299,6 +300,9 @@ def extract_grism_objects(input_model,
        Compute a wavelength array for the datamodel.  Computationally
        expensive, but saves doing it repeatedly later on in pipeline.

+    wfss_extract_half_heigh : int, (optional)


typo: missing the trailing "t" on height

hbushouse · 2020-07-19T16:15:57Z

jwst/assign_wcs/util.py

@@ -658,6 +677,17 @@ def create_grism_bbox(input_model,
                    xmax = int(np.max(xstack))
                    ymin = int(np.min(ystack))
                    ymax = int(np.max(ystack))
+                    if wfss_extract_half_height is not None:


Is the source catalog info for each source known at this point in the processing? This param value needs to be applied ONLY if the source being processed has is_star = True in the source catalog (only applied to point sources).

yes, the source catalog is available, will include this check

nden · 2020-07-19T17:49:34Z

I added the check for is_star with a made up value. This will need to be revised after the decision of how to populate the column. I also addressed the rest of the comments.

hbushouse · 2020-07-19T18:21:44Z

jwst/assign_wcs/util.py

+                    # The ``72`` criteria is a placeholder fof when there's a decision
+                    # on how to populate the ``is_star`` column in the catalog.
+                    # See ``is_star`` property in source_catalog.py
+                    if wfss_extract_half_height is not None and obj.is_star > 72:


Is there a way to test for NaN versus True, such that if NaN, don't apply, and if True, then do apply it.

hbushouse · 2020-07-21T13:05:59Z

docs/jwst/extract_2d/main.rst

-``--extract_orders``
-  list. The list of orders to extract. The default is taken from the ``wavelengthrange``
-  reference file.
+For NIRCam TSGRISM, the ``extract_2d`` step has one optional argument:

 ``--extract_height``


Should update this now to --tsgrism_extract_height

hbushouse · 2020-07-21T13:08:03Z

jwst/assign_wcs/tests/data/step_SourceCatalogStep_cat.ecsv

@@ -23,6 +23,7 @@
 # - {name: orientation_sky, unit: deg, datatype: float64}
 # - {name: isophotal_abmag, datatype: float64}
 # - {name: isophotal_abmag_err, datatype: float32}
+# - {name: is_star, datatype: float64}


Perhaps you should modify the data file used in the test to have the is_star column be Boolean and modify the entries in the ecsv file itself to also be boolean for that column (perhaps with a mixture of True and False values).

hbushouse · 2020-07-21T13:10:14Z

jwst/assign_wcs/util.py

@@ -659,6 +680,21 @@ def create_grism_bbox(input_model,
                    ymin = int(np.min(ystack))
                    ymax = int(np.max(ystack))

+                    # The ``72`` criteria is a placeholder fof when there's a decision


You can get rid of this comment now, since you've already changed obj.is_star to be used as a boolean.

hbushouse · 2020-07-21T13:13:26Z

jwst/lib/catalog_utils.py

@@ -118,6 +119,8 @@ class SkyObject(namedtuple('SkyObject', ("id",
        Upper left corner of the minimum bounding 
DE0F
box
    sky_bbox_ur : `~astropy.coordinates.SkyCoord`
        Upper right corner of the minimum bounding box
+    is_star : float


Need to change the data type to bool

nden added the Wide Field Slitless Spec (WFSS) label Jul 10, 2020

hbushouse added this to the Build 7.6 milestone Jul 10, 2020

hbushouse added the extract_2d label Jul 10, 2020

jdavies-st reviewed Jul 11, 2020

View reviewed changes

nden force-pushed the jp-1431 branch from aa43ee8 to 8ef1f83 Compare July 18, 2020 20:12

added a parameter wfss_extract_half_height to extract_2d

229d99b

add change log

nden force-pushed the jp-1431 branch from 8ef1f83 to 229d99b Compare July 18, 2020 20:45

nden requested a review from hbushouse July 18, 2020 21:26

nden added the ready-for-final-review label Jul 18, 2020

hbushouse reviewed Jul 19, 2020

View reviewed changes

address comments

f03fcf6

hbushouse reviewed Jul 19, 2020

View reviewed changes

nden added 3 commits July 20, 2020 14:08

changed is_star column in the source catalog to be of type bool

7589753

fix var name

9938ba6

Replace is_star column in catalogs with a boolen column

1f888db

hbushouse reviewed Jul 21, 2020

View reviewed changes

address comments

712cd05

hbushouse approved these changes Jul 21, 2020

View reviewed changes

nden merged commit ac33804 into spacetelescope:master Jul 21, 2020

stscijgbot-jp mentioned this pull request Jul 21, 2020

WFSS extraction size for point sources needs to be a fixed value that the user can set #4896

Closed

nden deleted the jp-1431 branch July 26, 2023 16:59

	extract_height: int
	Cross-dispersion extraction height to use for time series grisms.
	This will override the default which for NRC_TSGRISM is a set
	size of 64 pixels.

	def get_dispersion_direction(exposure_type, grating="ANY", filter_wh="ANY",
	pupil="ANY"):
	"""Get the dispersion direction.

JP-1431: add parameter extract_height to wfss extraction #5140

JP-1431: add parameter extract_height to wfss extraction #5140

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!