Add illustration_id import and Stage D illustration deduction calculator - #509
Merged
Conversation
Phase 1: Import Scryfall illustration_id into CanonicalPrintingMetadata - Add illustration_id UUIDField to PrintingMetadataRow and model - Add resolved_illustration_id property for single/double-face parsing - Migration 0089 for the new field Phase 2: Stage D illustration deduction calculator (issue #507) - IllustrationIndex: in-memory (artist_pk, card_name) → illustration_id → printing_pks - calculate_illustration_verdict: 0→abstain, 1→vote@0.85, N>1→union@0.85/N - run_illustration_calculator: batch runner with eligibility, gate, purgeability - Wired into management command between fallback and slow-path - Tests for index, verdict logic, integration, dry-run, gate check, purgeability
Source bug: - IllustrationIndex.artist_by_pk was keyed by artist_pk but match_artist expects card_pk keys; separated into a dedicated query over ALL CanonicalCards so artists are matchable even without illustration metadata Test bugs: - CanonicalArtistFactory.objects.get() — factory class, not a model - candidates=candidate (bare) instead of candidates=[candidate] - _join_key_no_hit_card set both printing and is_no_match=True, violating cardprintingtag_printing_xor_no_match constraint - _eligible_card had content_phash=None, causing all cards to be skipped - purge_machine_votes called with unsupported interactive kwarg
This was referenced Jul 28, 2026
WilfordGrimley
added a commit
that referenced
this pull request
Aug 4, 2026
modern_artist_credit.recognize_artist_credit lands in this PR as a third artist fallback inside compute_card_evidence, but without a version bump the fix reaches no existing row: Stage C's resume filter (MANIFEST_EXTRACTOR_CURRENT_VERSIONS, issue #509) compares stored extractor_versions against the manifest and skips any row whose value already matches, and all 230,378 production rows currently carry artist_ocr: artist-ocr-v2. Bumps ARTIST_OCR_EXTRACTOR_VERSION and its MANIFEST_EXTRACTOR_CURRENT_VERSIONS counterpart to artist-ocr-v3, with a dedicated version-history comment on the constant (the shared v1->v2 OCR-engine-swap comment above it now covers only collector_line_ocr/collector_line_tsv, which are not bumped here). Updates the three test fixtures asserting the literal "artist-ocr-v2" string to match.
7 tasks
WilfordGrimley
added a commit
that referenced
this pull request
Aug 5, 2026
… + CI totality (#690) Inventory + declare EXTRACTOR_OWNERSHIP for every module-private helper in image_evidence.py and every externally-imported callable it actually calls from collector_line_artist.py/local_ocr.py/local_image_quality.py that helps determine a stored ImageEvidence field but carries no *_EXTRACTOR_VERSION of its own (issue #509's own follow-up). All 17 entries are components of an already-versioned extractor - none are new extractors, so no manifest key, golden-set expectation, or stored value changes. - .github/scripts/check_extractor_ownership_totality.py: AST-derived totality check, sibling to check_extractor_manifest_sync.py. Derives the set of module-private helpers + called scoped-external imports reachable from compute_card_evidence's own call graph and fails CI if it disagrees with EXTRACTOR_OWNERSHIP in either direction. Cross-checks every declared owning key against the real MANIFEST_EXTRACTOR_KEYS (imported directly from check_extractor_manifest_sync.py). Excludes local_fallback.py and _collector_line_ocr_attempts (the OCR tier ladder) - both out of scope, worked on in parallel per this PR's own brief. - .github/scripts/tests/test_check_extractor_ownership_totality.py: fixture + real-repo tests, including a case proving a newly-added undeclared contributor fails CI. - .github/workflows/docs-lint.yml: wires the new tether in as its own job, mirroring extractor-manifest-sync exactly. - docs/features/catalog-completion-plan.md: "Stage C extractor ownership" section - the rule, why the two rosters stay in sync, the two explicit exclusions, and a correction to this PR's own brief (local_art_edge.py's extended-art classifier, cited as an example, is not actually reachable from compute_card_evidence today - it's a declared-not-yet-live Stage D calculator, not a Stage C extractor). - image_evidence.py: pointer paragraph in the module docstring; no behavioral change.
WilfordGrimley
added a commit
that referenced
this pull request
Aug 5, 2026
…on (#685) * fix(image_evidence): wire the artist-crop fallback into live extraction artist_ocr_name stays blank on 87,371 evidence rows that carry non-blank artist_ocr_raw_text - the "Illus." anchor misses OCR-noisy reads (e.g. "Soot Itus." never matches _ILLUS_RE), and the 2026-07-29 collector-line recovery (recover_artist_from_card_text) only reads collector_line_raw_text/ legal_line_raw_text, so it is structurally blind to old-border proxies whose only credit is a centred "Illus. <name>" line with no bottom print row at all. cardpicker.modern_artist_credit.recognize_artist_credit already exists for exactly this shape of input (re-reading artist_ocr_raw_text) but was only ever reachable through its own standalone backfill_modern_artist_names command, never during real extraction. This wires it into compute_card_evidence as a third fallback, tried only after both the anchor and the collector/legal-line recovery find nothing storable - strictly additive (new modern_artist_lexicon param, default None, no extractor_versions bump), and threaded through both live Stage C entry points (stage_e_dispatch.py, run_image_evidence_cohort.py). Verified against real production evidence rows (ids 221241/221268/221274): recognize_artist_credit recovers "Sebastian Giacobino"/"Aaron Miller"/ "Andrey Kuzinskiy" from their stored artist_ocr_raw_text at ratio 1.0 against the real ~2.5k-name lexicon. A fourth reproduction row (id 221289, "David Rapoza") turned out to be a correct abstention, not a bug: the real lexicon holds both "Dave Rapoza" and "David Rapoza" as distinct canonical artists, so the reading is genuinely ambiguous under the 2026-07-29 ruling (fuzzy matching yes, fuzzy storage no) - confirmed by direct measurement, not assumed. Full-population measurement of the 87,371-row split: 19,478 (22.3%) genuinely recoverable via the artist-crop fallback, 329 (0.4%) genuinely ambiguous (correctly abstained), 67,564 (77.3%) carry no artist credit findable by any means - consistent with this catalog's known proxy population rendered without standard bottom-row metadata. * fix(tests): thread modern_artist_lexicon through remaining compute_card_evidence stubs test_stage_e_shakedown.py and test_stream_full_catalog.py's _install_ok_stage_c_stub helpers mirrored compute_card_evidence's pre-modern_artist_lexicon signature, missed by the sibling repair in test_stage_e_dispatch.py and test_run_image_evidence_cohort.py (bf17be1) because neither module was run locally. Same fix, same shape: accept and ignore the new keyword-only param. * feat(question-feed): expose per-candidate art-crop URL on PrintingCandidate Adds PrintingCandidate.artCropUrl, sourced from the existing CanonicalPrintingMetadata.art_crop_url sidecar field (no new fetch or harvest - the data is already on disk), following the illustrationId precedent in shape and style: - CanonicalCard.serialise_as_printing_candidate populates it, null- tolerant for both the no-sidecar and empty-art_crop_url cases. - PrintingCandidate in both schema_types.py and schema_types.ts gains the optional field (backend from_dict/to_dict, frontend interface + typeMap entry). - docs/features/printing-tags.md's Known gaps section notes the field now exists but nothing consumes it yet - rendering it is future frontend work. Query efficiency: every path that reaches serialise_as_printing_candidate (get_ranked_printing_candidates's two branches, and the ai_vote lookup in question_feed._confirm_suggestion_item) already select_related's printing_metadata for illustrationId's sake, so no new prefetch wiring was needed. A new test proves the candidate-grid serialisation issues zero additional queries regardless of candidate count. This closes the backend half of the gap PR #687 (question-feed-frontend- fixes branch, not yet merged) identified: illustration-clustered candidate tiles have no shared art crop to render instead of each candidate's own full-card scan. Consuming this field in the frontend grid remains out of scope here. * feat(question-feed): render artCropUrl on illustration-clustered tiles; reconcile docs Item 1: merged origin/master (PR #685 was CONFLICTING) - the reconciled Known-gaps bullet takes the branch side (artCropUrl now exists, nothing consumed it) since it is the informed successor to master's #687 bullet. Item 2: illustration-clustered candidate tiles now render PrintingCandidate.artCropUrl in place of mediumThumbnailUrl, falling back to the printing scan when a candidate's metadata sidecar has none. Ungrouped/identify_printing tiles are unaffected. Item 4: the Known-gaps bullet this closes is removed; the shipped behaviour is documented on the existing 'Illustration grouping' architecture bullet instead. * feat(image-evidence): bump artist_ocr extractor to v3 (PR #685) modern_artist_credit.recognize_artist_credit lands in this PR as a third artist fallback inside compute_card_evidence, but without a version bump the fix reaches no existing row: Stage C's resume filter (MANIFEST_EXTRACTOR_CURRENT_VERSIONS, issue #509) compares stored extractor_versions against the manifest and skips any row whose value already matches, and all 230,378 production rows currently carry artist_ocr: artist-ocr-v2. Bumps ARTIST_OCR_EXTRACTOR_VERSION and its MANIFEST_EXTRACTOR_CURRENT_VERSIONS counterpart to artist-ocr-v3, with a dedicated version-history comment on the constant (the shared v1->v2 OCR-engine-swap comment above it now covers only collector_line_ocr/collector_line_tsv, which are not bumped here). Updates the three test fixtures asserting the literal "artist-ocr-v2" string to match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 — Import Scryfall illustration_id (Import Scryfall illustration_id into CanonicalPrintingMetadata (illustration deduction, phase 1) #506): Add illustration_id UUIDField to PrintingMetadataRow and CanonicalPrintingMetadata. Single-faced cards carry it at top level; snip double-faced cards nest it in card_faces[0]. Migration 0089 adds the indexed field.
Phase 2 — Stage D illustration deduction calculator (Stage D illustration deduction calculator (stage-d-illustration-v1) — phase 2 #507): New calculator module (stage-d-illustration-v1) that uses illustration_id relationships to deduce printing identity from artist-OCR hits. Uses in-memory index of (artist_pk, card_name) → illustration_id → [printing_pk]. 0→abstain, 1→vote at BASE_CONFIDENCE=0.85, N>1→union at BASE_CONFIDENCE/N. Wired between fallback and slow-path in the management command. DEDUCTION source with normal machine weight (no weight override).
Test plan
Scope note: confidence stays INFORMATIONAL per owner ratification — does not affect vote_consensus.py weight math. Zero-weight override scoped to deductive-backfill-v1 only. New calculator uses normal DEDUCTION weight. No vote pooling across illustration siblings.
Closes #506, Closes #507