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-41391: Add limits on the number of sources used for the matching kernel #290
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the code changes and checked the tests. Coverage doesn't drop and only minor changes requested.
Have you run a data processing check? I can if not.
"slot_ApFlux_flag", "slot_PsfFlux_flag", | ||
"base_PixelFlags_flag_interpolated", | ||
"base_PixelFlags_flag_saturated", | ||
"base_PixelFlags_flag_bad", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should flags associated with injected sources be used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, once we have switched over to using the new source injection we can rely on those source flags, and remove all of the checks on the mask plane. If you tell me the new flags to use, we can add them now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know exactly but I expect at least:
injected
and injected_core
flags, but I am figuring out how is this playing when we have injected from template ase well.
tests/test_subtractTask.py
Outdated
@@ -402,6 +402,52 @@ def test_few_sources(self): | |||
"Cannot compute PSF matching kernel: too few sources selected."): | |||
task.run(template, science, sources) | |||
|
|||
def test_kernel_source_selector(self): | |||
"""Test with only 1 source, to check that we get a useful error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this docstring is unaccurate. Add the correct description
4306415
to
5bab3ef
Compare
If more sources are available than the configured limit, select the highest signal-to-noise sources.
5bab3ef
to
431e9b2
Compare
dtype=float, | ||
default=500, | ||
doc="Maximum signal to noise ratio of detected sources " | ||
"to use for calculating the PSF matching kernel." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good parameter to have available. The default value is OK, and it would be best for each instrument to have this value configured accordingly.
If more sources are available than the configured limit, select the highest signal-to-noise sources.