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-29272: PTC task: Validate that the variance calculation provided by awf and the C_00 entry of the covariance matrix produced via FFT using Astier's code is the same #78

Merged
merged 7 commits into from Apr 6, 2021

Conversation

plazas
Copy link
Contributor

@plazas plazas commented Apr 1, 2021

No description provided.

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

It looks good overall. I just have some minor suggestions which I commented inline.

# Compare Cov[0,0] and afwMath.VARIANCECLIP
# covDiffAstier[0] is the Cov[0,0] element, [3] is the variance, and there's a factor of 0.5
# difference with afwMath.VARIANCECLIP.
thresholdPercentage = 1. # 1%
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to have this as a config field with a default value of 1.0 than have it hard coded here?

@@ -112,7 +112,7 @@ class PhotonTransferCurveExtractConfig(pipeBase.PipelineTaskConfig,
nIterSigmaClipPtc = pexConfig.Field(
dtype=int,
doc="Number of sigma-clipping iterations for afwMath.StatisticsControl()",
default=1,
default=3,
Copy link
Member

Choose a reason for hiding this comment

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

Should changes to default values be recorded in a CHANGELOG of some sort, somewhere? Probably not, but at the very least, this should be in its own commit. Same goes for the change in L105.

@plazas plazas merged commit 8a71ce0 into master Apr 6, 2021
@plazas plazas deleted the tickets/DM-29272 branch April 6, 2021 20:24
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