From 7282c76b6836002b011a97c1e74a92ab18b8a978 Mon Sep 17 00:00:00 2001 From: Holden Date: Tue, 16 Jun 2026 23:09:14 +0000 Subject: [PATCH] fix: address 5 code review findings (round 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - diversity: revert conf_array default from 0.5 back to 1.0 (np.ones); the 0.5 default caused None-confidence images to receive a 1.7× FPS boost and beat high-confidence detections — counter-productive for Frigate training data quality - diversity: fix hard_count to include None-confidence images (count images where score is None or < 0.85, not only confirmed < 0.85); the previous check systematically undercounted boosted images when the Immich faces API omits the score field - executor: fix garbled comment fragment "Skipped on / skipped when" left by a partial edit in round 4; merge into a single coherent sentence - executor: expand actually_uploaded trade-off comment to document all three consequences of a tracker write failure (Frigate duplicate, quality-replacement exclusion, cap-slot consumption), not only the duplicate risk mentioned previously - upload_tracker: add person_ids guard to reset_person isinstance check so the non-list warning only fires when cleanup would actually have run, not on no-op calls where person_ids is empty --- winnow/diversity.py | 10 +++++----- winnow/executor.py | 17 ++++++++++------- winnow/upload_tracker.py | 2 +- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/winnow/diversity.py b/winnow/diversity.py index 3c6ad8d..6961f56 100644 --- a/winnow/diversity.py +++ b/winnow/diversity.py @@ -517,9 +517,10 @@ def _cluster_aware_selection( emb_normed = emb_matrix / np.maximum(norms, 1e-8) # Build confidence weight array for hard example boosting. - # Default to 0.5 for faces with no confidence score so they receive a - # moderate diversity boost rather than being treated as high-confidence. - conf_array = np.full(n, 0.5) + # Default to 1.0 for faces with no confidence score: treat as high-confidence + # (no boost) rather than hard-example territory. A missing score field should + # not cause these images to beat genuinely high-confidence detections in FPS. + conf_array = np.ones(n) if confidence_scores: for i, c in enumerate(confidence_scores): if c is not None: @@ -586,8 +587,7 @@ def _cluster_aware_selection( 1 for i in selected if confidence_scores and i < len(confidence_scores) - and confidence_scores[i] is not None - and confidence_scores[i] < 0.85 + and (confidence_scores[i] is None or confidence_scores[i] < 0.85) ) logger.info("Selection complete: %s images (%s hard examples with confidence < 0.85).", len(selected), hard_count) diff --git a/winnow/executor.py b/winnow/executor.py index 07b72b6..52b7401 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -396,8 +396,8 @@ def upload_to_frigate(jobs: list[dict]) -> None: # Pre-upload Frigate score — clean measurement (image not yet in training set). # Called for all below-cap uploads (seeds frigate_scores for future at-cap - # replacement) and for at-cap uploads when scores already exist. Skipped on - # skipped when has_frigate_model is False (effective_count was 0 before the loop). + # replacement) and for at-cap uploads when scores already exist. + # Skipped when has_frigate_model is False (effective_count was 0 before the loop). # recognize_face returns (face_name, score); we only use the score when the # best match is for the correct person. Mismatches (or "unknown") are treated # as None so a wrong-person score never drives a ceiling skip or replacement. @@ -539,11 +539,14 @@ def upload_to_frigate(jobs: list[dict]) -> None: person_has_fscores = True # Always record for reconcile so the Frigate filename→asset_id # mapping is created even when the tracker write fails. - # Trade-off: if mark_uploaded failed, this asset_id is not in - # the upload set, so it may be re-selected next run (Frigate - # duplicate). The alternative — not appending — leaves the file - # permanently unmapped, breaking quality-replacement scoring. - # Frigate duplicate is the lesser consequence. + # Trade-off: if mark_uploaded failed, asset_id is absent from + # asset_ids and scores. Consequences: (1) re-selected next run + # → Frigate duplicate; (2) excluded from quality-replacement + # candidates (_pick_mapped_file requires a scores entry); + # (3) counted toward MAX_AUTO_IMAGES cap (via frigate_files). + # The alternative — not appending — leaves the file permanently + # unmapped (reconcile never creates the frigate_files entry), + # making (2) and (3) permanent. Frigate duplicate is lesser. actually_uploaded.append((fname, asset_id)) break diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 4e29abf..6997b6a 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -442,7 +442,7 @@ def reset_person(person_name: str) -> None: data["by_person"] = by_person flat_key = _flat_key(filename) person_ids = set(_get_ids(tracker_entry)) - if flat_key in data and not isinstance(data[flat_key], list): + if person_ids and flat_key in data and not isinstance(data[flat_key], list): logger.warning( "reset_person: %s has unexpected type for %s (%s) — skipping flat-list cleanup", filename, flat_key, type(data[flat_key]).__name__,