fix(metrics): use COCO eval index 8 for AR50_90 in MetricsMLFlowSink - #735
Conversation
|
|
@tillfri ^^ 🦝 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #735 +/- ##
======================================
Coverage 65% 65%
======================================
Files 56 56
Lines 7207 7207
======================================
+ Hits 4692 4693 +1
+ Misses 2515 2514 -1 🚀 New features to boost your workflow:
|
I have signed it, got an Internal Server Error earlier 😅 Signed it again, this time it worked (i think), hopefully it just takes some time to update :D |
|
@tillfri seems CLI was not still update, could you pls do it again 🙏 |
I have tried multiple times, with different devices, supposedly successful, but status is not changing😅 |
|
I see, and I guess that the problem is that the GH does not match your commitment to your profile |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect COCO Average Recall logging in the MLflow metrics sink (Issue #734) so that AR50_90 matches the same COCOeval stats entry used by the other logging sinks.
Changes:
- Update
MetricsMLFlowSinkto readAR50_90from COCOevalstats[8](AR @ maxDets=100) instead ofstats[6]. - Apply the same correction for EMA COCO eval metrics.
Comments suppressed due to low confidence (1)
src/rfdetr/util/metrics.py:395
- There’s no regression test covering the COCO
statsindex used forMetrics/*/AR50_90inMetricsMLFlowSink.update. Adding a small unit test that feeds a dummytest_coco_eval_bboxarray with distinct values at indices 6/8 and asserts that AR50_90 logs the index-8 value would prevent this from silently regressing again (ideally without requiring the realmlflowdependency, e.g., by monkeypatching the module).
ema_coco_eval = values["ema_test_coco_eval_bbox"]
ema_ap50_90 = safe_index(ema_coco_eval, 0)
ema_ap50 = safe_index(ema_coco_eval, 1)
ema_ar50_90 = safe_index(ema_coco_eval, 8)
if ema_ap50_90 is not None:
metrics_dict["Metrics/EMA/AP50_90"] = ema_ap50_90
if ema_ap50 is not None:
metrics_dict["Metrics/EMA/AP50"] = ema_ap50
if ema_ar50_90 is not None:
metrics_dict["Metrics/EMA/AR50_90"] = ema_ar50_90
| coco_eval = values["test_coco_eval_bbox"] | ||
| ap50_90 = safe_index(coco_eval, 0) | ||
| ap50 = safe_index(coco_eval, 1) | ||
| ar50_90 = safe_index(coco_eval, 6) | ||
| ar50_90 = safe_index(coco_eval, 8) | ||
| if ap50_90 is not None: |
There was a problem hiding this comment.
Consider avoiding hard-coded COCOeval stats indices here. This bug happened because the MLflow sink diverged from other sinks; defining named module-level constants (e.g., COCO_STATS_AP_50_95_IDX=0, COCO_STATS_AP_50_IDX=1, COCO_STATS_AR_50_95_MAX100_IDX=8) and reusing them across all sinks would reduce the chance of future mismatches and improve readability.
I fixed it by force pushing the same commit with --ammend --reset-author --no-edit, sorry for the inconvenience, i was not on my usual device |
What does this PR do?
fix Issue #734
Type of Change
Testing
Checklist
Additional Context