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-27437: Have maxMeanSignal (and minMeanSignal) be a list per amp, instead of a single number in the PTC task #63

Merged
merged 3 commits into from Nov 10, 2020

Conversation

plazas
Copy link
Contributor

@plazas plazas commented Nov 6, 2020

No description provided.

@plazas plazas requested a review from czwa November 6, 2020 17:52
@@ -101,14 +101,16 @@ class MeasurePhotonTransferCurveTaskConfig(pexConfig.Config):
default=1,
)
minMeanSignal = pexConfig.Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this use pexConfig.DictField instead? This won't have a clean command line interface (I think you need to use a config file to set DictField entries), but it would put the type checking into the config object instead of forcing some later parsing. The lack of a command line interface will also be mitigated by the switch to gen3.
An example of the config field: https://github.com/lsst/pipe_tasks/blob/master/python/lsst/pipe/tasks/ingest.py#L58
An example of setting the values: https://github.com/lsst/obs_lsst/blob/master/config/ucd/ingestCalibs.py#L14

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'll try this.

python/lsst/cp/pipe/ptc.py Show resolved Hide resolved
Copy link
Contributor

@czwa czwa left a comment

Choose a reason for hiding this comment

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

Took me a moment to find the default setting for min/maxMeanSigmaDict, but it looks fine now.

Modify docstring
@plazas plazas merged commit b30456c into master Nov 10, 2020
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