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-24756: Make sigma clipping configurable in PTC task #37

Merged
merged 4 commits into from May 13, 2020

Conversation

plazas
Copy link
Contributor

@plazas plazas commented May 12, 2020

No description provided.

@plazas plazas requested a review from czwa May 12, 2020 21:02
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.

Minor naming disagreement.

@@ -145,6 +145,11 @@ class MeasurePhotonTransferCurveTaskConfig(pexConfig.Config):
doc="Sigma cut for outlier rejection in PTC.",
default=5.0,
)
sigmaCutAfwMathStatsControl = 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.

This should probably be called somethingNumSigmaClip, so it's clear what the clipping is being applied to (the "something") and so it matches the name of the property set in the statsControl object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nSigmaClipPtc would, I think, be in keeping with our namings elsewhere.

@@ -145,6 +145,11 @@ class MeasurePhotonTransferCurveTaskConfig(pexConfig.Config):
doc="Sigma cut for outlier rejection in PTC.",
default=5.0,
)
sigmaCutAfwMathStatsControl = pexConfig.Field(
dtype=float,
doc="Sigma cut for afwMatch.StatisticsControl()",
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo I think: Match vs Math.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the name of the parameter to nSigmaClipPtc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...but I did fix the typo in the docstring, thanks!

sigmaCutAfwMathStatsControl = pexConfig.Field(
dtype=float,
doc="Sigma cut for afwMatch.StatisticsControl()",
default=3.0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we change this to 5.5 please?

Choose a reason for hiding this comment

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

On None and use the default if not set

@plazas plazas merged commit 89544ce into master May 13, 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

4 participants