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

Updated/corrected and the wf3rej.rst and wf3rej.py files #71

Merged
merged 2 commits into from
Apr 24, 2023

Conversation

mdlpstsci
Copy link
Contributor

Both the "readthedocs" documentation, as well as the wf3rej.py routine were updated/corrected. Removed background documentation from wf3rej.py as this text belongs in the docs. Updated/added examples for both Python and C executable usage. Provided explanatory text as to the actions taken by the underlying program when the options use their default values.

@mdlpstsci mdlpstsci added bug maintainance General Package Mantainance documentation labels Oct 6, 2022
@mdlpstsci mdlpstsci self-assigned this Oct 6, 2022
@mdlpstsci mdlpstsci changed the title Updated/corrected and the wf3rej.rst and wf3rej.py files Do Not Merge until HSTCAL > v2.7.4 is released: Updated/corrected and the wf3rej.rst and wf3rej.py files Oct 12, 2022
and WFPC2 data (`crrej`), providing a well-tested and robust procedure.

First, `wf3rej` temporarily removes the sky background from each input image
(if requested via the SKYSUB in the CRREJTAB or by a parameter passed
Copy link
Collaborator

Choose a reason for hiding this comment

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

i would reword this slightly for clarity: (if specified via the SKYSUB parameter in the CRREJTAB, or by the parameter passed to the Python...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* a Python list of filenames
* a partial filename with wildcards (``\*raw.fits``)
* filename of an ASN table (``\*asn.fits``)
* a partial filename with wildcards (``*flt.fits``)
Copy link
Collaborator

Choose a reason for hiding this comment

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

so you can't pass in an ASN table? that input option was removed from the docs here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The option of ASN was removed because it was not actually supported by the underlying C code.

Rejection propagation threshold.

badinpdq : int, default=0
Data quality flag bits to reject.

crmask : bool, default=False
If True, flag CR in input DQ images.
crmask : bool, default=Setting to be read from CRREJTAB.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the default if not specified in CRREJTAB? I imagine all of these parameters have coded defaults that are overridden by CRREJTAB and then by user input?

Copy link
Contributor Author

@mdlpstsci mdlpstsci Apr 17, 2023

Choose a reason for hiding this comment

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

Values for the optional input parameters are expected be specified on the command line or resident in the CRREJTAB. If the parameters are not specified AND the CRREJTAB does not exist or has some other issue (i.e., missing the column for badinpdq), the executable reports an error and stops. In this case there are NO default values in the source code which are then replaced with CRREJTAB or command line input.


shadcorr : bool, default=False
If True, perform shading shutter correction.
shadcorr : bool, default=Setting to be read from SHADCORR keyword value in primary header of first image to process.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same also applies here. i think the docstring should have whatever the coded default is if there is one (in this case, seems to be none) and a note that header values will take precedence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a calibration step keyword which has values of PERFORM, SKIPPED, or OMIT. This is set during the creation of the RAW files from the POD file source and keyword rules in Operations (or at least before it gets to the calibration pipeline software). A user can override the setting as desired. The specific setting of the *CORR calibration step keyword is dependent aspects of the data being process, and (again) is controlled by the "rules" external to the calibration pipeline software.

array and is ignored when images are summed together. Surrounding pixels
within some expansion radius (CRRADIUS) are marked as ‘SPILL’ pixels and
are given less stringent detection thresholds.
Background discussion cn the wf3rej algorithm can be found in the following locations:
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo, cn > on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

- a Python list of filenames
- a partial filename with wildcards (``*raw.fits``)
- filename of an ASN table (``*asn.fits``)
- a partial filename with wildcards (``*flt.fits``)
Copy link
Collaborator

Choose a reason for hiding this comment

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

just clarifying the change here - this function does not take in associations anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function actually could never process ASN tables as the underlying C code does not support this feature.

Copy link

@FDauphin FDauphin 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 besides a few minor suggestions. Also curious about the _asn.fits not being an argument anymore.

within some expansion radius (CRRADIUS) are marked as ‘SPILL’ pixels and
are given less stringent detection thresholds.
Background discussion cn the wf3rej algorithm can be found in the following locations:
https://wfc3tools.readthedocs.io/en/latest/wfc3tools/wf3rej.html,

Choose a reason for hiding this comment

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

and Section 3.4.5 of the WFC3 Data Handbook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


for image in infiles:
if not os.path.exists(image):
raise IOError("Input file not found: {0}".format(image))

# Generate a comma-separated string of the input filenames
input = ','.join(infiles)

call_list.append(input)

if output:

Choose a reason for hiding this comment

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

For the lines below, raising errors instead of printing statements and returning errors will cut a few lines (not necessary just a little cleaner)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Removed background documentation from wf3rej.py as this text
belongs in the docs.  Updated/added examples for both Python
and C executable usage. Provided explanatory text as to the
actions taken by the underlying program when the options use
their default values.
@mdlpstsci
Copy link
Contributor Author

Appropriate code review suggestions addressed in 180ba7d.

@mdlpstsci mdlpstsci changed the title Do Not Merge until HSTCAL > v2.7.4 is released: Updated/corrected and the wf3rej.rst and wf3rej.py files Updated/corrected and the wf3rej.rst and wf3rej.py files Apr 17, 2023
@mdlpstsci
Copy link
Contributor Author

@cshanahan1 @FDauphin I am starting to prep for a new build release candidate which can include as many WFC3 changes that can fit. This PR needs a re-review. Thanks.

@FDauphin
Copy link

FDauphin commented Apr 21, 2023

@cshanahan1 @FDauphin I am starting to prep for a new build release candidate which can include as many WFC3 changes that can fit. This PR needs a re-review. Thanks.

@mdlpstsci The review was requested for @spacetelescope/wfc3tools-maintainers (which I do not think I have access to) instead of my profile.

Copy link

@FDauphin FDauphin left a comment

Choose a reason for hiding this comment

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

My comments were addressed. Approving for merge; thanks for getting this up to date 🙌🏾

@mdlpstsci mdlpstsci merged commit 8014cf1 into spacetelescope:master Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug documentation maintainance General Package Mantainance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants