DM-12943: Add utilities for detection when noise is correlated. #96
Conversation
TallJimbo
force-pushed
the
tickets/DM-12943
branch
from
1d614c2
to
00f40c1
Compare
src/CorrelatedNoiseDetection.cc
Outdated
| if ((*mi.getMask())(x1, y1) & badBitMask) { | ||
| continue; | ||
| } | ||
| float z = (*mi.getImage())(x1, y1) / (*mi.getVariance())(x1, y1); |
There was a problem hiding this comment.
Don't you need to subtract the mean here and below from the image values?
There was a problem hiding this comment.
My thinking was that because this should only be running noise pixels, we could assume the mean was zero. I suppose I should at least add a comment noting that we should check that assumption if we ever use this code in a pipeline setting.
There was a problem hiding this comment.
I found that there were many regions where this was not the case in the HSC wide data.
There was a problem hiding this comment.
Of course, they may just get filtered out when taking the median over a tract.
There was a problem hiding this comment.
Just started thinking about fixing this, and I realized that I don't understand what you're proposing - the "mean" we need to subtract is a correction to the background estimate, right? How do we estimate that?
There was a problem hiding this comment.
Yes, depending on how large the image is the background estimate may be off. Can't you just compute the mean of the image and subtract that off?
There was a problem hiding this comment.
Yup, if you think just subtracting off the image mean would be an improvement (I was imagining something more local), that's definitely something I can do.
src/CorrelatedNoiseDetection.cc
Outdated
| if (miIter.mask() & badBitMask) { | ||
| continue; | ||
| } | ||
| *outIter += z*miIter.image(); |
There was a problem hiding this comment.
Why are you dividing the variance value from only the first pixel? Shouldn't be something like sqrt(var(x1,y1))*sqrt(var(x2,y2))?
There was a problem hiding this comment.
I think you're right - I was originally thinking of this as part of the stationarity assumption, but I don't need to assume the variance is stationary to assume that the correlations are.
| class CorrelatedNoiseDetectionTestCase(lsst.utils.tests.TestCase): | ||
| """Test utilities for dealing with correlated noise when running detection.""" | ||
|
|
||
| def testMeasureCorrelatationKernel(self): |
There was a problem hiding this comment.
Your test here doesn't use correlated values. I'm wondering if this is a useful test? I could imagine getting the correlation values correct for this simple case, but missing a bug that would get the correlation wrong when there actually is correlation.
There was a problem hiding this comment.
Yup, this was just laziness, because I didn't want to work out how to add correlated stationary noise with NumPy. If you happen to have a recipe for that on hand, I'd love to have it.
There was a problem hiding this comment.
I gave this a shot, and unfortunately the naive way to do it with NumPy (constructing a full covariance matrix) is totally impractical - if we want enough pixels in the noise image to be able to estimate the kernel, the covariance matrix is too big to even allocate. I could imagine working around that with sparse matrices, and I'm sure there's a way to do this with an FFT, but at this point it's not looking like it's worth the effort.
There was a problem hiding this comment.
Yes, FFT is the way to go. I could potentially give this a shot today. It is conceptually simple, but it always takes me a while to get all the factors correct.
There was a problem hiding this comment.
That'd be great. I'm not in any hurry to merge this, so please don't feel like you need to get it done today.
Would it make sense to just make that a separate ticket (maybe to add more reusable functionality to afw::math::Random), and block the merge of this ticket on that one?
There was a problem hiding this comment.
Yeah, that sounds like a good idea.
There was a problem hiding this comment.
No description provided.