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-40451: Allow deblending to continue even when data is missing for some bands #72

Merged
merged 3 commits into from Sep 26, 2023

Conversation

fred3m
Copy link
Contributor

@fred3m fred3m commented Sep 13, 2023

Previously any footprints that could not construct a PSF in all bands were skipped. As part of HSC PDR4, because there are patches with only partial coverage of narrow bands (for example), we want to allow the deblender to proceed.

This ticket implements deblending with only partial PSF coverage, and also adds new columns to the catalog to keep track of the coverage per band.

@fred3m fred3m changed the title Allow deblending to continue even when data is missing for some bands DM-40451: Allow deblending to continue even when data is missing for some bands Sep 13, 2023
Previously any footprints that could not construct a PSF in all
bands were skipped. As part of HSC PDR4, because there are patches
with only partial coverage of narrow bands (for example), we want
to allow the deblender to proceed.

This ticket implements deblending with only partial PSF coverage,
and also adds new columns to the catalog to keep track of the
coverage per band.
Copy link

@cmsaunders cmsaunders left a comment

Choose a reason for hiding this comment

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

This looks good. I have a few comments about places where I think you could make the logic more straightforward, but it's your call whether you make those changes.

python/lsst/meas/extensions/scarlet/io.py Show resolved Hide resolved
python/lsst/meas/extensions/scarlet/scarletDeblendTask.py Outdated Show resolved Hide resolved

try:
psfModels = mExposure.computePsfKernelImage(psfCenter)
except IncompleteDataError as e:

Choose a reason for hiding this comment

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

My general feeling is that try-excepts should be avoided as much as possible. Since you are already changing the parameters and returned values in computePsfKernelImage, you could consider adding something like an allowMissing option, then always returning the psfs and the filters used. I'm not sure if that would be simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a good example of a case where it is easier to ask forgiveness than acceptance. I originally used an allowMissing parameter but it requires an additional check by the user to see if any bands were missing. Since this check is already being done while creating the PSF image I wanted to find a way to avoid having to validate the output again, and this seemed like a good way to ensure that users who don't expect a bad PSF will get an error, and users who know that it might happen do not have to check for it.

Choose a reason for hiding this comment

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

Ok, sounds reasonable to me.

python/lsst/meas/extensions/scarlet/scarletDeblendTask.py Outdated Show resolved Hide resolved
tests/test_deblend.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/scarlet/scarletDeblendTask.py Outdated Show resolved Hide resolved
…dels

Because deepCoadd_forced does not require the imageMask to set the
coverage column (which instead is in the meas catalog),
this ticket makes that parameter optional.
@fred3m fred3m merged commit 3c46264 into main Sep 26, 2023
2 checks passed
@fred3m fred3m deleted the tickets/DM-40451 branch September 26, 2023 19:44
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