perf(mcbyte): skip mask-only work without mask evidence - #573
perf(mcbyte): skip mask-only work without mask evidence#573JESUSROYETH wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Optimizes McByte’s common mask-disabled association path by skipping unnecessary mask candidate construction.
Changes:
- Adds an early return when usable mask evidence is unavailable.
- Refactors mask-boost inputs and adds regression coverage.
- Documents the performance improvement.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/trackers/core/mcbyte/mask_association.py |
Skips mask-only candidate work when evidence is unavailable. |
tests/core/test_mcbyte_mask_association.py |
Tests six no-evidence states and input immutability. |
CHANGELOG.md |
Records the optimization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This looks good; let's re-format it as a table and ensure that the HOMTA/MOTA metrics do not regress |
|
mcbyte isn't in
Ran it twice in the same tree, Updated the Measured section to a table too. |
There was a problem hiding this comment.
Hi @JESUSROYETH! 👋
Thank you for your valuable contribution! It overall looks very good!
I thoroguly checked your PR and I have some remarks. Here below and in the code. Could you reply to and/or resolve all of them? Thanks!
Remarks:
1.
the mask-disabled path is the common one, not an edge case
Just for clarity, this is the default setting as the heavy mask dependencies are not installed with pip install trackers (one needs to perform pip install trackers[mask] to install the mask support).
Normally though, McByte needs the mask for its desired behavior. Otherwise, it behaves similarly to BoT-SORT.
2.
I ran the same method by hand: feed ground-truth boxes into McByteTracker(enable_cmc=False, enable_mask_manager=False)
So if I understand, you re-run McByte with your changes applied, but only without the mask enabled? Do you have the results with masks enabled? I mean HOTA, IDF1 and MOTA. We also need to see these before proceeding. Notably, the difference before and after your changes.
3.
“I ran the same method by hand: feed ground-truth boxes into McByteTracker(enable_cmc=False, enable_mask_manager=False)
What do you mean by ground-truth boxes specifically? The track ground truth data without track IDs? Or actual detections? Normally, we prefer the result comparison with detections, e.g. coming from YOLOX as used for our benchmarks on test splits. Detections are imperfect and thus can expose more performance differences when modifying the tracker's code, while ground truth bounding boxes are idealized and might not exposed as much.
4.
Every value above came out identical to the decimal.
That's very good and well appreciated. Please keep doing it 🙂
…onstruction Addresses review feedback on the mask-only skip. _apply_mask_similarity_boosts takes MaskOutput again instead of the three unpacked fields, and narrows the two optional ones (masks, mask_avg_prob_dict) inside the helper. tracklet_mask_dict is not optional, so it never needed unpacking. condition_similarity_with_masks no longer builds MaskConditionedAssociation before the boosts run. The no-evidence path builds and returns its own result, and the mask path builds it after the boosts have been applied, so the in-place mutation happens on a plain local array rather than through an already constructed frozen dataclass. Output is unchanged: both paths compared bit-identical against the previous revision over 5000 randomised cases, with the caller matrix untouched.
|
Hi @tstanczyk95 👋 Thank you very much for the review, it was really helpful. The points about how to frame the change and how to validate it are fair, and I will keep this pattern in mind for the next PRs. On 1, you are right and my wording was loose. Saying "the mask-disabled path is the common one" is true about a plain On 3, by ground-truth boxes I meant the annotation boxes fed in as if they were detections, with the track IDs dropped. Masks were off in that run too. Your objection is correct: that makes it an oracle-detection setup, and therefore less sensitive to association changes than imperfect detector output. It was not a strong enough validation for the concern you raised. On 2, I ran it again the way you asked, with masks enabled and real detections, using the repo's own benchmark instead of a script of mine: To check run-to-run stability, I ran the baseline twice. Both repetitions matched at full metric precision and produced the same seven SHA-256 hashes. The PR run then produced those same hashes.
All seven per-sequence results also matched, and the seven MOTChallenge output files were byte-identical. COMBINED HOTA is 0.5073214955745664 in both runs if you want the full precision. Per-sequence, identical on both arms
One thing worth stating, since an all-equal table can also mean the mask path never ran: I instrumented I should be clear about one substitution: these are the public FRCNN detections the repo hosts for MOT17-val, not YOLOX. The YOLOX numbers you use are on the test splits through Codabench, which I cannot score locally, and I did not find YOLOX detection files for the val split in the repo. These FRCNN detections still exercise the concern you raised: imperfect detections with misses and false positives instead of idealised boxes, and anyone can reproduce it with one Also, the absolute values above are not comparable with the published MOT17 table, they use a different split and a different detector. They are only meant for the before/after comparison. |
tstanczyk95
left a comment
tstanczyk95
left a comment
There was a problem hiding this comment.
Hi @JESUSROYETH,
Great, thank you for your detailed answer and explanations. It is well appreciated!
@Borda It looks good to me now, it can be merged.
McByte defaults to
enable_mask_manager=False, so the mask-disabled path is the common one, not an edge case. Its three association stages still ran into mask-only candidate construction, though:condition_similarity_with_masks()copied and reduced the full similarity matrix, built the ambiguity and optional isolated-candidate matrices, and only after that checked whether there was usable mask evidence. On a default tracker those matrices got built for nothing, then discarded.The fix
The function keeps the same order as before: input validation, clear-match locking, reduced assignment. It now returns right there, before mask-only candidate construction starts, when no tracklet can actually receive mask evidence. That covers a few cases: the output or mask array is missing, the mask array has zero length, or the tracklet-to-mask or confidence mapping is empty or missing. With usable mask evidence, candidate construction and score updates stay the same as before.
Measured
For each dataset five of seven pairs came out faster and two slower. The ranges overlap, so I'd call the end-to-end numbers supportive but machine-dependent, not a clean win.
Correctness
5,000 random mask/no-mask cases give the exact same bit-level digest before and after, with zero mutations of the caller's matrix. I also compared a real MOT17-04 run, pairing 15,161 boxes by IoU (not by index) between the two implementations: minimum IoU 1.0, zero coordinate-bit or tracker-ID mismatches.
Tests
A regression test covers the six no-evidence states, checks the candidate builders don't get called, and verifies both the locked/reduced result and that the caller's matrix stays untouched. Mask-association suite: 40 passed. Full non-integration suite: 1,554 passed, 3 skipped, 14 deselected. Pre-commit clean (ruff, formatting, codespell, mypy).