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-32700: Add a task to compute noise correlations #294

Merged
merged 3 commits into from Sep 20, 2022

Conversation

arunkannawadi
Copy link
Member

No description provided.

@arunkannawadi arunkannawadi force-pushed the tickets/DM-32700 branch 2 times, most recently from 8ca85b3 to f91033e Compare September 16, 2022 19:45
Copy link

@fred3m fred3m left a comment

Choose a reason for hiding this comment

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

I might be misunderstanding the code, but I don't think that background subtraction is working properly (see comments in the PR). Either way, there should be a test to make sure that includes masked pixels and background subtraction to verify that it's working correctly and to prevent bitrot.

Comment on lines +85 to +99
def setDefaults(self):
self.background.binSize = 32
self.background.useApprox = False
self.background.undersampleStyle = "REDUCE_INTERP_ORDER"
self.background.ignoredPixelMask = [
"DETECTED",
"DETECTED_NEGATIVE",
"BAD",
"SAT",
"NO_DATA",
"INTRP",
]
Copy link

Choose a reason for hiding this comment

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

Should this use the same mask planes as the variance maskPlanes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that variance is estimated largely from the background, that seems reasonable to me, especially in the defaults. Of course, there's nothing in the code that requires them to be the same. I used the ones listed in ScaleVarianceTask as a starting point here.

Copy link

Choose a reason for hiding this comment

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

Is this the proper way to do this (I'm asking honestly here, as I don't know the proper way to set defaults for a subtask that might be different than the subtasks default config)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is certainly a proper way. The defaults here override the defaults set in SubtractBackgroundTask and they can be overridden by specifying different config at the runtime. As to whether the default values make sense, that's simply motivated by ScaleVarianceTask which has been used on HSC and DeCam analyses, so I'm running with that.

python/lsst/meas/algorithms/noise_covariance.py Outdated Show resolved Hide resolved
python/lsst/meas/algorithms/noise_covariance.py Outdated Show resolved Hide resolved
@arunkannawadi arunkannawadi force-pushed the tickets/DM-32700 branch 5 times, most recently from 9540119 to ee4ad06 Compare September 20, 2022 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants