Closed stscijgbot-jp closed 5 months ago
Comment by Alicia Canipe on JIRA:
Instrument team reps: can you think of any issues with this update? We can have a more in-depth discussion if needed.
Misty Cracraft David Law Bryan Hilbert Kevin Volk Jo Taylor James Muzerolle Maria Pena-Guerrero
Comment by Howard Bushouse on JIRA:
Yes, you explained it correctly Alicia Canipe. Now that the jwst pipeline can make use of parameter ref files, there's really no need for this dedicated drizpars ref file and only makes it confusing to users, developers, and the flow of the actual step code!
Comment by Misty Cracraft on JIRA:
I'll add that no instrument currently has a parameter reference file for resample. Yes, the system can handle them, but we have to make them and get them into the system first. Once those are in place, I think I agree with dropping the drizpars file, that I think was made as an example file by SCSB and not the instrument teams.
Comment by Bryan Hilbert on JIRA:
Makes sense to me, as long as users have access to all the parameters through the parameter reference file. Someone working offline will still be able to set pixfrac to something other than 1.0 via a custom parameter reference file?
Comment by Howard Bushouse on JIRA:
Bryan Hilbert yes, right now the drizpars ref file only contains settings for pixfrac, kernel, fillval, and weight_type, all of which are already accessible via step params and hence can be set via individual overrides or via a pars-resamplestep ref file.
Comment by Kevin Volk on JIRA:
As far as I can see this should not be an issue for NIRISS. The only question that comes to mind is whether all the parameters of interest are available in the resample parameter file? I think the answer is yes and so there should not be any problem with this. Is there any instance where the parameters for the resampling of the individual images in the MAST needs different parameters than when other uses of drizzle happen? That seems unlikely to me.
Comment by Howard Bushouse on JIRA:
We now have at least 2 other tickets directly related to the use of the drizpars reference file and how it's causing problems for some things (see JP-2580 and JP-3360). I think it's time we had some action on this. The comments above make it sound like there's consensus on deprecating the use of the drizpars ref file and having the teams deliver pars-resamplestep ref files instead. That also allows the opportunity to review the default settings for all instruments and modes and optimize them based on real-world in-flight experience (something that's never been done for drizpars). Can we have a DMS WG discussion on this? I don't think it requires Cal WG discussion, because it's not algorithm related (it's functional).
Comment by Melanie Clarke on JIRA:
I was not aware of this proposal, but I like it as a solution to the various problems with the drizpars file. The drizpars format is cumbersome, and removing it will simplify the code and its use.
Hi all, NIRISS has been discussing this a bit more and we are concerned about losing the ability to define the drizzle parameters as a function of number of input images. As I understand it, moving to a resample par file would remove this ability. NIRISS imaging is primarily used 1) in parallel with another instrument, and Nimages can vary quite a lot as a result, or 2) as part of the WFSS mode and Nimages is usually <4. For case 1) having the parameters as a function of Nimages could be useful.
However, I am mostly interested in hearing how losing this ability would affect NIRCam, as they are a prime imaging instrument and users could have very large Nimages for some programs. Alicia Canipe Bryan Hilbert what are your thoughts?
Comment by Howard Bushouse on JIRA:
After doing some investigation into the question of whether it's possible to use a parameter reference file to provide multiple parameter values based on the number of input images being processed (as is possible with the current drizpars ref file setup), it appears that the answer is no.
What we're doing now is exploring whether it would be possible to have multiple pars ref files in CRDS, one each for different values of the number of input images, and figure out a way to select the right one when the resample step is executed.
If that also turns out to be not possible, I think we have 2 options as to how to proceed:
1) Keep the drizpars ref files as is, so that folks can have parameter values based on the number of input images and just fix the resample/resample_spec steps to no longer override the values contained in the drizpars ref file.
2) Switch over to using pars-resamplestep ref files (and do away with drizpars ref files) and just accept the fact that there's no way to have control over the param values based on the number of images being combined. Personally I think this would be acceptable, especially for the automated pipeline environment. Users who have specific science cases that could take advantage of something like custom pixfrac
values could do so by reapplying the step on their own and supplying custom param values. I think that the idea of basing pixfrac
simply on the number of input images is a fragile and risky approach anyway. It's not just the number of images that's important, but also the kind of sub-pixel dither pattern that's in use. It would be completely inappropriate, for example, to use pixfrac<1.0 when you have several images that are spread out into mosaic tiles, with no sub-pixel dithering.
Comment by Howard Bushouse on JIRA:
Adding some comments here that've taken place in threads in other tickets and/or jwst pull requests, so that all discussion is documented for historical purposes.
James Davies commented:
The logic based on number of input images in rows in the old drizpars fits binary table only
really varied pixfrac:$ showtable jwst_nircam_drizpars_0001.fits
numimages filter pixfrac kernel fillval wht_type stepsize
--------- ------ ------- ------ ------- -------- --------
1 ANY 1.0 square INDEF exptime 10
2 ANY 1.0 square INDEF exptime 10
4 ANY 0.8 square INDEF exptime 10
but this was never tested or optimized and is from 2006 when resample was first implemented.
These are all placeholders. Anything other than pixfrac=1.0 in fact is not actually
supported mathematically. It's the equivalent of unsharp mask in Photoshop™. stepsize
is not used, exptime weighting instead of ivm ruins the uncertainties in output error arrays
and the errors in the source catalogs, and INDEF is meaningless.
How about we make proper, tested input parameter sets as reference files that are not
tables and do not depend on number of input images? I'll add that if there's new pars-reffile
to be delivered, this is an excellent time to get rid of all references to values of
"INDEF" in this codebase (referring to the resample/resample_spec steps) as well, which
have no meaning in Python or the C++ code in drizzle. See https://github.com/spacetelescope/jwst/issues/2219 and
https://github.com/spacetelescope/jwst/issues/7664.```
[Tyler Pauly](https://jira.stsci.edu/secure/ViewProfile.jspa?name=tpauly) commented 5-Feb-2024:
https://github.com/spacetelescope/jwst/pull/8258 🙈
Do we have a write-up somewhere (I thought I'd seen it detailed in a github issue discussion from 3+ years ago) explaining this (the problem with downstream errors when weight_type='exptime' is used) to users?```
Comment by Howard Bushouse on JIRA:
James Davies further commented on 19-Feb-2024:
I would argue that having the output resample scale (arcsec per pixel) change or pixel
shrinking on the input images before drizzling in standard DMS processing based on number
of input images is not what you want.
First, you don't know the dither pattern based just on the number of images, and you don't
know the science use case. Different dither patterns will sample the pixel phase differently.
The number of overlapping images will be different than the number of images in the
modelcontainer (NIRCam long vs short), and in the end, anything other than pixfrac=1.0
(standard geometric overlap distribution of the input flux to the output flux) or pixfrac=0
(uncorrelated noise in output) is essentially doing Photoshop unsharp mask on the images.
Yes, you can do it, but it is not scientifically supported in terms of its effect on
the PSF, correlated uncertainties, etc in the output image. It makes strong assumptions
about the distribution of flux within the pixel which is simply not correct unless you
know something about the shape (gradient of flux) in the underlying source on sky.
Currently the above drizpars reffile shrinks pixels (pixfrac < 1), but there is no change
the output scale - complete garbage, and not what anyone would do for real science.
If anything, one does not shrink pixels (pixfrac=1) but does rescale the pixel size of the
output to match to some other band, say to match NIRCam short and long on the same pixel scale.
The whole reason people played with pixfrac to begin with is because HST pixels severely
undersampled the PSF, and for stars, one knows something about the PSF. This is not the
case for NIRCam and extragalactic fields. And while we can argue about its validity in
fiddling with pixfrac for JWST NIRISS and NIRSpec, which are undersampled, I don't think
anyone has shown the effects of such assumptions on the chunky JWST PSF. In the end, best
not to go there and instead forward-model whatever it is you're trying to do.
The standard pipeline should not assume particular science scenes (stars, galaxies,
exoplanets) for general processing, but should produce products that are those that are
best understood and best calibrated, and for this, no input pixel shrinking and no pixel
scaling in the output make the most sense, as we currently do.
Scientists are always free to disagree and reprocess their data with custom pipeline
parameters, as they all already do.```
Comment by Bryan Hilbert on JIRA:
The NIRCam team has discussed this, and we are in favor of removing the drizpars file in favor of the resample parameter reference file. We agree with the points James and others made above, namely that the number of images alone is not enough to determine a proper value of pixfrac/output scale to use. Knowledge of the exact dither pattern/mosaic tiles is also necessary. Attempting to fine tune parameter values would entail a lot of work on the part of the instrument teams in order to find the proper values, and even then we would be making assumptions about what the users want for their science. We think it makes more sense to keep pixfrac=1 for the automated pipeline (as is done for HST). Users who are interested in output products at different pixels scales can then manually re-run the resample step with their preferred parameter values.
Comment by Howard Bushouse on JIRA:
It sounds like most, if not all, instrument teams are on-board with making the switch from drizpars reference files to pars-resample (or pars-resamplespec) parameter reference files. The use of a pars-resamplespec ref file for NIRSpec has already been tested and proven to do the desired thing of overriding and ignoring the hardwired default parameter values in the step code (see discussion in JP-3360).
So at this point we simply need the instrument teams to create and deliver pars-resample/pars-resamplespec ref files. These could be delivered to CRDS-TEST, in order to test against the current code and verify that the desired behavior is happening. Unfortunately we just passed our deadlines for trying to get this into B10.2, but given that the param ref file deliveries by themselves will result in changes in behavior (with no need for code updates), the new ref files could be delivered and installed in Ops whenever they're ready to go (i.e. we don't need to wait for the B11.0 deadline).
Comment by Rachel Plesha on JIRA:
Jo Taylor I know this was still a concern for NIRISS. Has there been any update since your last comment in January?
Comment by Bryan Hilbert on JIRA:
Howard Bushouse Rosa Diaz what does the schema for this file look like, and what are the CRDS selection criteria? Are you anticipating one file per instrument? Mostly I'm wondering if I need to worry about exp_type/p_exp_ty, or will CRDS not be paying any attention to that?
Howard Bushouse The NIRISS team discussed this again and based on the rationale and comments above, we agree that the deprecating this file is fine.
Comment by Howard Bushouse on JIRA:
Bryan Hilbert We don't have schema for parameter reference files (only for calibration reference files). Basically the "schema" is set by the list of step parameters that are defined in the step code itself. And the selection criteria can be anything you want, as specific or general as you want, i.e. only select on instrument and useafter, or also EXP_TYPE, etc. The only catch of course is that any selectors that you choose have to have an entry in the param ref file. For example, because the existing NIRCam param ref file for the image2 pipeline uses TSOVISIT as a selector, there's an entry for TSOVISIT in the meta data block of the ref file (see https://jwst-crds.stsci.edu/browse/jwst_nircam_pars-image2pipeline_0002.asdf).
I believe the schema here (that is pretty lax, so likely less helpful than the example) is used by stpipe for to_asdf
and from_asdf
for the step configs:
https://github.com/spacetelescope/stpipe/blob/main/src/stpipe/resources/schemas/step_config-1.0.0.yaml
Thanks. So it sounds like selection criteria haven't been defined yet. I think we can get away with a single file that applies to all modes.
Comment by Meaghan McDonald on JIRA:
Hi all. Just became aware of this ticket and catching up. Right now, we only have a CRDS ticket (CRDS-784) filed for implementing the pars-resamplespecstep reference file for NIRSpec. For the teams planning to deliver pars-resamplestep and/or pars-resamplespecstep reference files, we will need CRDS tickets filed for each reference type, for each instrument, that detail reference file information in the form on this page.
These reference types are already defined in the imaps, so we would just need to create and deliver the initial rmaps before any reference files could be delivered. Any CRDS code updates would be tagged for build 11.0.
Bryan Hilbert Jo Taylor Misty Cracraft James Muzerolle Alicia Canipe David Law
Comment by Misty Cracraft on JIRA:
If imaging and LRS agree that we want the same values set in the parameter reference file, can we only define a resample file, or do we need to also have a resamplespec file with the same values set? Just trying to figure out what the code might need.
Comment by Howard Bushouse on JIRA:
Misty Cracraft There are actually two different top-level steps that get called: resample and resample_spec, even though further down they eventually call the same drizzling routines, and hence you will need to have a pars-resamplestep and pars-resamplespecstep ref files, even if they contain the same params.
Comment by Misty Cracraft on JIRA:
Ok, one more question. Since MIRI will need two files, one for resample and one for resample_spec, do I need to create two separate CRDS tickets for those, or can I combine them in one ticket to request new filetypes?
Comment by Meaghan McDonald on JIRA:
I would say for organizational purposes, let's do two separate tickets. That way there will be no rmap confusion when it comes time to deliver. Misty Cracraft
Comment by Misty Cracraft on JIRA:
CRDS tickets created, with sample files attached. Linked to this ticket as CRDS-801 and CRDS-802.
Comment by Meaghan McDonald on JIRA:
Thank you Misty Cracraft . We now have CRDS tickets for MIRI, NIRCam and NIRSpec. Should we expect NIRISS tickets for these files? Jo Taylor
Comment by Rachel Plesha on JIRA:
Apologies for the delay. The new parameter reference file for NIRISS imaging is now at CRDS-807.
Comment by Rachel Plesha on JIRA:
Howard Bushouse is there a github PR for this to test these files before June 5th as discussed at today's meeting?
Comment by Howard Bushouse on JIRA:
Rachel Plesha You should be able to test the pars file using any version of jwst, including the released 1.14.0 (B10.2) or the latest master branch. All we need to do is verify that when you supply a resample/resample_spec pars ref file to the step it ignores any settings in the existing drizpars ref file and uses the param settings from the new resample/resample_spec pars ref file instead.
Comment by Rachel Plesha on JIRA:
Howard Bushouse With my new file (which is on CRDS-807):
parameters: {class: jwst.resample.ResampleStep, name: resample, numimages: 1, filter: ANY, pixfrac: 1.0, kernel: square, fillval: INDEF, wht_type: exptime, stepsize: 10} ```
copied directly from the drzpars file:
```java
numimages [1]
filter ['ANY']
pixfrac [1.]
kernel ['square']
fillval ['INDEF']
wht_type ['exptime']
stepsize [10] ```
I'm getting the following error:
```java
Traceback (most recent call last):
File "/Users/rplesha/miniconda3/envs/jwst/lib/python3.11/site-packages/stpipe/cmdline.py", line 332, in just_the_step_from_cmdline
step = step_class.from_config_section(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/rplesha/miniconda3/envs/jwst/lib/python3.11/site-packages/stpipe/step.py", line 280, in from_config_section
config_parser.validate(config, spec, root_dir=dirname(config_file or ""))
File "/Users/rplesha/miniconda3/envs/jwst/lib/python3.11/site-packages/stpipe/config_parser.py", line 378, in validate
raise ValidationError("\n".join(messages))
stpipe.config_parser.ValidationError: Extra value 'numimages' in rootThe above exception was the direct cause of the following exception:Traceback (most recent call last):
File "/Users/rplesha/miniconda3/envs/jwst/lib/python3.11/site-packages/stpipe/cli/strun.py", line 24, in main
Step.from_cmdline(sys.argv[1:])
File "/Users/rplesha/miniconda3/envs/jwst/lib/python3.11/site-packages/stpipe/step.py", line 191, in from_cmdline
return cmdline.step_from_cmdline(args)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/rplesha/miniconda3/envs/jwst/lib/python3.11/site-packages/stpipe/cmdline.py", line 383, in step_from_cmdline
step, step_class, positional, debug_on_exception = just_the_step_from_cmdline(
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/rplesha/miniconda3/envs/jwst/lib/python3.11/site-packages/stpipe/cmdline.py", line 341, in just_the_step_from_cmdline
raise ValueError(str(e)) from e
ValueError: Extra value 'numimages' in root```
With the same error for numimages, filter, wht_type, and stepsize, before finally running through without any of those four parameters included.
Perhaps I'm just calling the wrong class in my file? But I assume these should still be valid parameters that are accepted? I'm using jwst version 1.14.0.
Comment by Howard Bushouse on JIRA:
Rachel Plesha The resample/resample_spec steps do not have a param named "numimages", so it's unrecognized and causing the error you see when the pipeline tries to parse the contents of your pars-resample file. The params that are available for the resample/resample_spec steps are:
pixfrac = float(default=1.0)
kernel = string(default='square')
fillval = string(default='INDEF' )
weight_type = option('ivm', 'exptime', None, default='ivm')
output_shape = int_list(min=2, max=2, default=None) # [x, y] order
crpix = float_list(min=2, max=2, default=None)
crval = float_list(min=2, max=2, default=None)
rotation = float(default=None)
pixel_scale_ratio = float(default=1.0) # Ratio of input to output pixel scale
pixel_scale = float(default=None) # Absolute pixel scale in arcsec
output_wcs = string(default='') # Custom output WCS.
single = boolean(default=False)
blendheaders = boolean(default=True)
allowed_memory = float(default=None) # Fraction of memory to use for the combined image.
in_memory = boolean(default=True) ```
Comment by Howard Bushouse on JIRA:
Rachel Plesha Also, in order to have different param values used for different filters (if that's what you want), you'll have to create separate pars files for each filter and have "FILTER" be used as a selector in CRDS for finding the right pars file to use for a given pipeline run. If you don't need filter-dependent values, then a single pars file is sufficient.
Comment by Rachel Plesha on JIRA:
Thanks Howard Bushouse! Seeing the accepted parameters is helpful. I see now earlier in this ticket there was discussion that the number of images parameter is to be dropped to use this file, and that stepsize was never used in the drizpars file. Filter makes sense, and I can updated wgt_type to weight_type.
Comment by Rachel Plesha on JIRA:
Testing by the NIRISS team shows that this works as expected on imaging data. The pars-resample values are used rather than those in the drizpars file, and the results are identical to the current version of the pipeline if no parameters are modified in the pars-resample file.
Comment by Howard Bushouse on JIRA:
I can confirm that when running the calwebb_image2 pipeline on a single NIRCam image, while pointing to CRDS-TEST context 1230, the resample step uses the param values that are set in jwst_nircam_pars-resamplestep_0001.asdf (e.g. weight_type=exptime instead of ivm). But the log still also includes the message
2024-06-04 15:11:30,233 - stpipe.Image2Pipeline.resample - INFO - Using drizpars reference file: ██████████████████████████████████████████████████████████████████ ```
which could be confusing to users. So we should probably go ahead with code updates to eliminate that message.
Comment by Howard Bushouse on JIRA:
Ditto for processing a MIRI image through calwebb_image2 and the resample step (now uses param values from pars-resamplestep ref file).
Comment by Howard Bushouse on JIRA:
Further testing of pars-resample*step ref files submitted to CRDS-TEST shows that all of them appear to work from a mechanical point of view. I did, however, notice what appear to be a couple of odd settings for some params in some of the new files.
1) jwst_nircam_pars-resamplestep_0001.asdf has "fillval=INDEF", which is the old default value for that parameter. The default value in the step code was recently changed (see https://github.com/spacetelescope/jwst/pull/8488) to "NAN", so that resampled pixels that have no contribution from the inputs are set to NaN, instead of zero. Using "fillval=INDEF" in the pars ref file will override that code default and go back to the old behavior.
2) jwst_niriss_pars-resamplestep_0001.asdf not only has "fillval=INDEF" (as above), but also has "weight_type=ivm". Are you sure you don't want to be using "weight_type=exptime"?
Comment by Bryan Hilbert on JIRA:
Yeah, NIRCam noticed the INDEF issue as well. We generated the file long enough ago that the INDEF/NaN issue was not on our radar, and we missed updating it before delivering the file to TEST. Can we deliver an updated version to TEST? Or should we wait and then deliver the updated version to OPS once everything moves over there?
Comment by Howard Bushouse on JIRA:
Note for record keeping that the completion of this ticket (i.e. activation of resample/resample_spec parameter ref files in CRDS) will also close JP-3360 and JP-2580.
Comment by Howard Bushouse on JIRA:
Rachel Plesha One other thing I noticed while running tests is that the NIRISS pars-image2pipeline ref file (in both CRDS and CRDS-TEST) for regular imaging mode has the resample step set to be skipped, so that the image2 pipeline only creates "cal" products, without any "i2d" products for individual exposures. Is that intentional? Just curious.
Comment by Kevin Volk on JIRA:
A few comment for NIRISS:
(1) yes we do want the weighting to the "IVM" rather than "exptime" as according to Paul Guidfrooij the latter produces bad results with interacting with the ramp shortening imposed by the charge migration step.
(2) While I have never liked the use of "NaN" values, we should set things to match the default in this instance.
(3) I was not aware that we do not produce level 2 resampled images for NIRISS. We certainly used to have these products. These are not vital once the level 3 products are produced since those are better, but they should be produced anyway. I do not remember what was discussed in producing the level 2 overall parameter file, so it may be that this was something that happened by accident rather than by design.
Comment by Kevin Volk on JIRA:
Concerning the level 2 resampled images, we might have also decided within the NIRISS team that the level 2 resampled images are not needed as products because they are not useful once the level 3 products are available. Thinking about things further, I have some memory that we were seeing the bad pixels get "magnified" in their effects in the resampling of the individual images because there is no other image available to fill in these values and they were affecting any pixel in the resampled image with overlap. I will have to ask some of the people on the team about this before we can decide whether the products should be turned on again.
Comment by Rachel Plesha on JIRA:
The exact same thing here with NIRISS about INDEF and NaN as NIRCam–I did not connect the dots between the multiple tickets.
Would it be better to leave that parameter out of the file so that we use the default rather than specifying that it is NaN Kevin Volk ? I can update the file to either remove the parameter or to change it to be NaN.
Comment by Kevin Volk on JIRA:
Are we filling in all the parameters for the resample step, or just a sub-set? If the latter, then it seems better to remove the parameter that sets the fill value for cases with no input data to drizzle to the output image.
Comment by Howard Bushouse on JIRA:
Rachel Plesha The preferred convention is to leave out params that you want to use the default value from the step code. Only those params for which you want to use non-default values should appear in the pars ref files. That way if/when the default value in the code gets changed, it won't get overridden by the (old) pars ref file. So yes, in this case it would be best to leave out "fillval" if you're OK with using the new default value of "NaN".
Comment by Bryan Hilbert on JIRA:
Howard Bushouse following that logic, it looks like the only parameter that would be in the pars-resamplespec file would be the weight_type = 'exptime', yes? From what I see, pixfrac defaults to 1, the kernel defaults to 'square', and fillval (will shortly) default to 'NaN'.
Comment by Howard Bushouse on JIRA:
Bryan Hilbert You are correct, sir.
Comment by Rachel Plesha on JIRA:
Howard Bushouse in that case, NIRISS is using all of the default values, as weight_type=ivm is the default, too I believe. What is the best plan of action then assuming we still need a file delivered?
Comment by Howard Bushouse on JIRA:
Rachel Plesha I think you should still go ahead and deliver a pars ref file for NIRISS, with only "weight_type=ivm", even though that is currently still the default, just so the pars ref file gets used by the resample step and we can discontinue any and all use of drizpars. If the ivm setting really does work better for NIRISS, then it's OK to to have the ref file force it to use that, in case the default in the step were to ever change to exptime.
Issue JP-2682 was created on JIRA by Alicia Canipe:
Based on discussion at the DMSWG meeting today about JP-2580 (notes), we would like to investigate the possibility of discontinuing the use of the drizpars reference file now that we have resample parameter files. Previously, the benefit of the drizpars file was that it allows multiple entries for a given observing mode, e.g., it can change the values of pixfrac depending on how many images are being combined. We can't do that with a parameter reference file; however, the CalWG made a decision to only use pixfrac=1, meaning we shouldn't need that functionality any more (right Howard Bushouse ? Is that the only parameter with that feature being used?) This would simplify things for developers and the pipeline users.
Anton Koekemoer Swara Ravindranath Misty Cracraft FYI