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-39138: Fix cp_verify test failures #26

Merged
merged 2 commits into from
May 17, 2023
Merged

DM-39138: Fix cp_verify test failures #26

merged 2 commits into from
May 17, 2023

Conversation

czwa
Copy link
Collaborator

@czwa czwa commented May 10, 2023

Comparing the image statistics against one realization of the read noise isn't correct.

@czwa czwa requested a review from erykoff May 10, 2023 21:27
@erykoff
Copy link
Contributor

erykoff commented May 15, 2023

I'm sorry, I don't totally follow what is going on here. A couple lines are deleted but I guess the values come from elsewhere...?

@czwa
Copy link
Collaborator Author

czwa commented May 15, 2023

Previously the code was setting the read noise value used in the test threshold from the as-measured serial overscan residual stdev. These values are in general smaller than the camera defined read noise values, resulting in cases where the measured image noise is smaller than the camera defined read noise, but more than 5% higher than the overscan residual and are marked as failures. I had originally thought this was fine, as the overscan residual stdev is a measurement of the read noise, but I don't think it's valid to do anymore. I think I have three reasons why a fixed read noise is better:

  1. The overscan data is used to determine the overscan fit that is subtracted off, so that standard deviation of that residual should be the smallest possible value.
  2. We know the per-row/per-column overscan subtraction adds some noise to the image, and forcing the test to fail because that additional noise isn't constrained by the residual stdev doesn't make sense.
  3. This is also just one realization of the amplifier read noise, so depending on the size of the true scatter in the read noise, we may be forcing the test to fail because we're using a value in the tail of that scatter. From the ComCam results posted today, it looks like there's 5-10% scatter in the measured overscan residual stdev, which means this may be the largest factor in determining how many amplifiers pass with this threshold.

Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

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

Okay, I think I see what's going on, that verify['NOISE'] is the test but verify['READ_NOISE_CONSISTENT'] is just a check?
I think the comments above the tests could be improved based on what you posted in the PR, and make it clear which "test below (that does not trigger failure)" refers to, because there are a lot of tests below.

@czwa
Copy link
Collaborator Author

czwa commented May 16, 2023

Significantly updated comments, and I've separated the advisory check in a way that should make it clearer that it is not a critical test.

Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

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

This is much clearer, thanks. One last question: previously, READ_NOISE_CONSISTENT was always filled, and now it's only filled if we have RESIDUAL_STDEV. Just want to confirm that this is okay.

@czwa
Copy link
Collaborator Author

czwa commented May 17, 2023

That was intentional, yes. Without the metadata we can't make any claims about the read noise matching, so I felt it was better to just drop it rather than assign any value.

@erykoff
Copy link
Contributor

erykoff commented May 17, 2023

That's what I figured, and I guess this isn't a required field so nothing will fall down if it's missing?

@czwa czwa merged commit 913f575 into main May 17, 2023
2 checks passed
@czwa czwa deleted the tickets/DM-39138 branch May 17, 2023 19:17
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