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

DM-21950: Update validate_drp to work with different external calibrations #108

Merged
merged 3 commits into from Dec 6, 2019

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Nov 7, 2019

Update validate_drp so that external photometric and astrometric corrections are separated, and so that fgcmcal can be used in place of jointcal for photometry.

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

Thanks for adding the exitStatus to __call__. Please squash the two commits related to that, since they go together.

externalPhotoCalibName = Field(
dtype=str,
doc=("Type of external PhotoCalib. Currently supported are jointcal, "
"fgcm, and fgcm_tract."),
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it should be a ChoiceField, to get some automatic validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't know about the ChoiceField.

python/lsst/validate/drp/matchedVisitMetricsTask.py Outdated Show resolved Hide resolved
python/lsst/validate/drp/matchedVisitMetricsTask.py Outdated Show resolved Hide resolved
python/lsst/validate/drp/matchedVisitMetricsTask.py Outdated Show resolved Hide resolved
python/lsst/validate/drp/matchreduce.py Outdated Show resolved Hide resolved
python/lsst/validate/drp/matchreduce.py Outdated Show resolved Hide resolved
python/lsst/validate/drp/matchreduce.py Outdated Show resolved Hide resolved
python/lsst/validate/drp/matchreduce.py Outdated Show resolved Hide resolved
python/lsst/validate/drp/validate.py Show resolved Hide resolved
@erykoff erykoff force-pushed the tickets/DM-21950 branch 3 times, most recently from 53c9def to 6810312 Compare December 3, 2019 16:56
Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

A handful of other comments here on some docs. Clean those up and you're good to merge.

doApplyExternalPhotoCalib = Field(
dtype=bool, default=False,
doc=("Whether to apply external photometric calibration via an "
"`lsst.afw.image.PhotoCalib` object. Uses the "
Copy link
Contributor

Choose a reason for hiding this comment

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

two spaces? ;-)

https://xkcd.com/1285/

Also, I don't think these docstrings need to be wrapped at 80 characters. At least, I hope they don't.

Parameters
----------
butler: `lsst.daf.persistence.Butler`
dataId: Butler dataId
Copy link
Contributor

Choose a reason for hiding this comment

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

Docstrings for these two (and below)? Is the "Butler dataId" a dict or an lsst.daf.persistence.ButlerDataRef?

Use jointcal/meas_mosaic outputs to calibrate positions and fluxes.
doApplyExternalPhotoCalib : bool, optional
Apply external photoCalib to calibrate fluxes.
Default is False.
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need to specify defaults here (and below).

… be used.

Configuration options have been updated to allow different external photometric
calibrations (e.g. fgcmcal) to be used.  The external PhotoCalib and WCS
datasets can now be specified separately as well.
External wcs is now doApplyExternalSkyWcs and externalSkyWcsName.
@erykoff erykoff merged commit a6117da into master Dec 6, 2019
@erykoff erykoff deleted the tickets/DM-21950 branch December 6, 2019 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants