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-48468: Turn off gaussian histogram Ks testing by default. #291

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Jan 22, 2025

No description provided.

@erykoff erykoff requested a review from jchiang87 January 22, 2025 15:48
Copy link
Contributor

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

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

I think this is fine as-is, aside from updating the doc for doKsHistMeasurement, but I'd also consider setting the p-values to 1.0 instead if that config is false.

@@ -201,6 +201,12 @@ class PhotonTransferCurveExtractConfig(pipeBase.PipelineTaskConfig,
'FULL': 'Second order correction.'
}
)
doKsHistMeasurement = pexConfig.Field(
dtype=bool,
doc="Do the Ks test of gaussianity? If False, will be filled with all 0.0; "
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to state explicitly that it's the ks test pvalues that will be filled with zeros. I'm wondering though if it would make more sense to fill them with 1.0 so that all of the points will be accepted no matter what pvalue threshold is used.

@erykoff erykoff merged commit 5dc75b4 into main Jan 22, 2025
2 checks passed
@erykoff erykoff deleted the tickets/DM-48468 branch January 22, 2025 20:46
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.

2 participants