DM-17431: Respect priority order when merging a footprint #451
Conversation
There was a problem hiding this comment.
Looks great!
One advantage of this "minimal change" version is that it's also not an ABI change, though the benefit of that mostly goes away after a new weekly comes out, so if this lands today that's not a big deal. I'm certainly happy for you to go with the bigger-change, more readable version of this if you like it better, but I think if we really wanted to make this code readable we'd need to make much more drastic changes, such as explicitly using SpanSet and PeakCatalog for the data members of FootprintMerge and only combining them into Footprints when we're done; right now it seems like we always have to work around the fact that a Footprint has both because we do different things with the spans and the peaks.
| if (doMerge && first) { | ||
| // Now merge footprint including peaks into the newly-connected, higher-priority FootprintMerge | ||
| first->add(foot, _peakSchemaMapper, keyIter->second, minNewPeakDist, maxSamePeakDist); | ||
| } else { |
There was a problem hiding this comment.
I assume doMerge == bool(first) is guaranteed? If so, is it worth asserting on that?
There was a problem hiding this comment.
As written no, that's not guaranteed. See new line 272. bool(first) says there was at least one overlapping FootprintMerge. doMerge is the input parameter that says "yes, merge it." Both have to be true for us to merge it. This logic I didn't change, but I also found it confusing, and why I was proposing some reordering (to move the doMerge up and out of the loop). Because what's the point of looping over existing FootprintMerges if you don't plan on merging them! Your comment demonstrated that doing that would be worthwhile. Going to put it on a different commit.
yalsayyad
force-pushed
the
tickets/DM-17431
branch
from
39738dd
to
62c321d
Compare
|
@TallJimbo I added the two additional commits I wanted to make. Will you take a look? Checked there's no behavior change and I'm rerunning Jenkins now. |
src/detection/FootprintMerge.cc
Outdated
| // If list is empty don't check for any matches, just add all the objects | ||
| bool checkForMatches = (_mergeList.size() > 0); | ||
| // If list is empty or merging not requested, don't check for any matches, just add all the objects | ||
| bool checkForMatches = (_mergeList.size() > 0) && doMerge; |
There was a problem hiding this comment.
Prefer !_mergeList.empty(); this comparison will generate a signed vs. unsigned warning on some compilers.
yalsayyad
force-pushed
the
tickets/DM-17431
branch
from
62c321d
to
1be0b4c
Compare
that overlaps previously disconnected higher priority footprints. Include unit test that simulates this case.
No behavior change.
to allow FootprintMergeList call it directly to avoid adding footprints twice (safe) and improve readability. Update inline comment describe adding SpanSets rather than whole Footprints
yalsayyad
force-pushed
the
tickets/DM-17431
branch
from
1be0b4c
to
8927949
Compare
that overlaps previously disconnected higher priority footprints.
Include unit test that simulates this case.