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-30172: Define BFK tests for cp_verify #8

Merged
merged 10 commits into from Dec 9, 2021
Merged

DM-30172: Define BFK tests for cp_verify #8

merged 10 commits into from Dec 9, 2021

Conversation

czwa
Copy link
Collaborator

@czwa czwa commented Nov 10, 2021

This adds catalog support to cp_verify, and then uses that to compare two ISR runs: one with and one without brighter-fatter correction. If the sizes trend in the correct direction, then the correction has improved things and is likely good.

examples/cpVerifyBfk.ipynb Show resolved Hide resolved
pipelines/VerifyBfk.yaml Show resolved Hide resolved
python/lsst/cp/verify/mergeResults.py Show resolved Hide resolved
python/lsst/cp/verify/mergeResults.py Show resolved Hide resolved
python/lsst/cp/verify/mergeResults.py Show resolved Hide resolved
python/lsst/cp/verify/verifyBfk.py Show resolved Hide resolved
python/lsst/cp/verify/verifyBfk.py Outdated Show resolved Hide resolved
python/lsst/cp/verify/verifyBfk.py Outdated Show resolved Hide resolved
python/lsst/cp/verify/verifyBfk.py Show resolved Hide resolved

catalogVerify['BRIGHT_SLOPE'] = False
catalogVerify['NUM_MATCHES'] = False
# These values need justification.

Choose a reason for hiding this comment

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

People should be rewarded for leaving comments like this, rather than punished, but reading it I still can't really help but ask... so, what's the justification for these values? (sorry!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NUM_MATCHES > 10 exists because we need to be able to trust the fit. In any real science image, this should be trivially met (the majority of my tests had ~200-300).
BRIGHT_SLOPE < -0.5 is entirely arbitrary. alpha must be negative (smaller size difference at larger magnitudes/fainter fluxes), but making it be simply negative ignores that the size change should be significant. If alpha =~ -0.01 we're essentially doing no correction. Every other constraint I can think if is circular, because it requires trying to figure out how much smaller the kernel should make things.
I am open to any suggestion on this.

Choose a reason for hiding this comment

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

Yeah, I don't really have any great suggestions, just had to comment on the comment really. Maybe just pasting this into a comment and replacing the # These values need justification. line would do it. And that way, if there's verifications failing in future, we can see that perhaps these values should be adjusted, as opposed to tests failing. (The other potential problem I can imagine is that if there's not much BF present in the first place (due to low depth in the observations, for example), or different sensors, (because we're camera-agnostic, haha), then this could cause a fail here, so noting that these values shouldn't be taken as gospel would be good).

@czwa czwa merged commit 371fd4a into main Dec 9, 2021
@czwa czwa deleted the tickets/DM-30172 branch December 9, 2021 21:57
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