DM-33710: Update to update footprints with scarlet lite models #210
Conversation
There was a problem hiding this comment.
Some minor comments.
Changes here reminded me what bad shape this task was already in, largely due to Gen2-Gen3 coexistence (which we can now clean up), but also the unfortunately-named refCatInBand=deepCoadd_meas connection and the almost-as-bad measCat=deepCoadd_forced_src local variable. Time to write an RFC, because fixing some of that is an API change.
| default=True, | ||
| doc="Whether to strip footprints from the output catalog before " | ||
| "saving to disk. " | ||
| "This is usually done when using scarlet models to save disk space.") |
There was a problem hiding this comment.
If you keep the current behavior:
| "This is usually done when using scarlet models to save disk space.") | |
| "This is usually done when using scarlet models to save disk space. " | |
| "If footprintDatasetName != 'scarletDeblendModels' then this field is ignored.") |
But I actually think this is a good option to have even when not using Scarlet models, and in fact it should be the default (though that might cause too much trouble downstream to do on this ticket) - these HeavyFootprints are always exactly the same as those stored in deepCoadd_meas, right?
There was a problem hiding this comment.
Yes. I checked out a meas and forced_src catalogs for the same patch and every heavy footprint was the same, which are the same as the ones in the deblendedFlux catalog when using either scarlet or the SDSS-like deblender catalog. So we shouldn't be storing them in meas or forced_src regardless of the deblender used, and I can make that change in this ticket for coadds.
| "IDs are compatible with reference source IDs" % | ||
| (srcRecord.getId(), self.config.coaddName, | ||
| self.config.footprintDatasetName)) | ||
| srcRecord.setFootprint(fpRecord.getFootprint()) |
There was a problem hiding this comment.
You had a comment above that said that attachFootprints was Gen2-specific and wasn't worth modifying (which sounds plausible, but isn't something I can confirm or deny off the top of my head). Did that turn out not to be true?
There was a problem hiding this comment.
That wasn't my comment, but one that has been in the repo since Nate updated this task 3 years ago. I think his point is that this method loads the footprint file from the data ref, which is not the way that gen3 works. So I think he was saying that when we officially deprecate gen2 we can remove this method, but in the meantime I wanted it to work with the scarlet outputs for consistency.
No description provided.