DM-44319: Refactor deblending in DetectAndMeasureTask #316
Conversation
e481cd7 to
8af121f
Compare
parejkoj
left a comment
There was a problem hiding this comment.
One concern is that downstream processing will have to filter on something to remove the "multiple parent copies". I don't know if we should also do that on this ticket, but we maybe should. I think that would be in FilterDiaSourceCatalogTask in ap_assocation. I don't know if we need to do it elsewhere, too; having both parents and children in the diaSource catalogs is something we're not fully familiar with yet.
| merged_footprints.makeSources(sources) | ||
|
|
||
| # Sky sources must be added before deblending, otherwise the | ||
| # sources with parent == 0 will be out of order and SFM cannot run. |
There was a problem hiding this comment.
We're not really running "SFM" here; maybe this instead sources with parent == 0 will be out of order and measurement will fail.?
There was a problem hiding this comment.
I'll change to "measurement tasks." The point is that measurement plugins require all of the children with the same parent ID to be contiguous, but the old version of this task added the sky sources after deblending, so if deblending had worked as expected then measurement would fail.
There was a problem hiding this comment.
I am a bit surprised by that, because I think that deblending was working on "isolated" positive blends before (it was just messing up other things), and we didn't see failures.
There was a problem hiding this comment.
Yeah, it's actually because before it wasn't doing any deblending, so all of the sources had parent==0. The measurement framework has a call to SourceCatalog.getChildren(0), where the argument is the parent ID. This is done to run all of the measurements on the parents, but in the new catalog if the sky objects were appended to it then the records with parent == 0 was discontinuous.
|
|
||
| # Update the deblended parents and add their children | ||
| # to the sources catalog. | ||
| sources.extend(temp_cat[first_child_index:], deep=False) |
There was a problem hiding this comment.
I think you want to do a deep copy here to ensure the catalog is contiguous, but maybe that's not necessary for the measurement tasks that are run in after this? And then the catalog is written to disk. I guess we can see if we get failures when running on larger data?
There was a problem hiding this comment.
No, we actually don't want a deep copy (see Jim's comments in slack). The temporary catalog actually created a view to the original records, that way the source IDs stay in order and the original records are updated in place.
There was a problem hiding this comment.
I checked and verified that using this procedure the sources are always contiguous.
| # Set detection and primary flags | ||
| for sid in skipped_ids: | ||
| src = sources.find(sid) | ||
| self.deblend.skipParent(src, difference.mask) | ||
| self.setPrimaryFlags.run(sources) |
There was a problem hiding this comment.
I understand what setPrimaryFlags is doing here, but I'm not familiar with skipParent. Could you please expand the comment above, since I don't think we're setting any "flags" here, but rather the "not deblended" mask plane (I think?)?
There was a problem hiding this comment.
Sure. The basic idea is that the deblender already has a method to set all of the appropriate flags for a parent source when deblending is skipped, so we just call that method to set those flags here.
There was a problem hiding this comment.
I think calling it "detection" as the comment does isn't quite correct, though, since we've already done detection. Maybe Set blending-related mask planes and flags on the non-deblended parents. would be more clear?
There was a problem hiding this comment.
Yes, see my latest commit. But for the setPrimary section I still use this comment because technically all of the flags set by isPrimary are detect_<flagName>, since they are about detection.
|
Couple more comments about comments, otherwise this looks good, thank you |
The previous deblending algorith was not properly deblending, as it was attempting to use meas_deblender for use cases that were not algorithmically suitable. This commit only runs the deblender on blends that have all positive peaks, the regime that it is designed for.
Revert "Merge pull request #316 from lsst/tickets/DM-44319"
The previous deblending algorith was not properly deblending, as it was attempting to use meas_deblender for use cases that were not algorithmically suitable.
This commit only runs the deblender on blends that have all positive peaks, the regime that it is designed for.