Schema Migration: Shared FluorState MTI for protein and dye states, introduce FluorescenceMeasurement

[tlambert03]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 82.81879% with 128 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.95%. Comparing base (efbdf23) to head (50aa2d7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
backend/proteins/models/fluorophore.py	71.42%	46 Missing ⚠️
backend/proteins/admin.py	62.50%	15 Missing ⚠️
backend/proteins/schema/types.py	72.72%	12 Missing ⚠️
backend/proteins/models/protein.py	87.17%	10 Missing ⚠️
backend/proteins/models/efficiency.py	57.14%	9 Missing ⚠️
backend/proteins/api/serializers.py	25.00%	6 Missing ⚠️
backend/proteins/tasks.py	72.22%	5 Missing ⚠️
backend/proteins/util/maintain.py	44.44%	5 Missing ⚠️
backend/proteins/forms/spectrum.py	87.87%	4 Missing ⚠️
backend/proteins/views/spectra.py	33.33%	4 Missing ⚠️
... and 7 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #387      +/-   ##
==========================================
+ Coverage   60.67%   61.95%   +1.27%     
==========================================
  Files         110      114       +4     
  Lines        8786     9008     +222     
==========================================
+ Hits         5331     5581     +250     
+ Misses       3455     3427      -28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[tlambert03]

Areas Not Fully Tested (Manual Verification Recommended)

1. Admin Inline for State - Does editing a State through Protein admin still work?
2. Spectrum submission form - Test submitting a new dye spectrum
3. Microscope efficiency reports - Run a full scope report with mixed proteins/dyes
4. GraphQL mutations (if any) - Verify they handle the new model structure
5. Algolia indexing - If you use Algolia, verify the index still works
6. Celery task calc_fret() - Check it still works with the new structure

[tlambert03]

@claude please re-review

[claude]

Claude finished @tlambert03's task —— View job

---

Re-review Complete: Schema Migration PR

Comprehensive re-review completed. All critical issues from the previous review have been addressed.

Tasks:

• Review current state of rebuild_attributes() method
• Check if measurement validation has been added
• Review responses to Copilot feedback
• Check for any new issues
• Provide updated assessment

---

Summary of Changes Since Last Review

Based on commit history and code review, the following improvements have been made:

✅ Critical Issues Resolved

1. rebuild_attributes() Primary Reference Sorting - FIXED (commit df635b8)

• The method has been completely refactored with proper priority logic
• Now correctly sorts by: trusted → primary reference → most recent
• Uses _get_primary_reference_id() helper that works for both Protein and Dye owners
• Implements waterfall logic with pinned field support
• Lines 126-181 in backend/proteins/models/fluorophore.py:126-181

2. Copilot Review Issues - FIXED

✅ SwitchingChoices enum labels corrected (commit f7c2a98)

• TIMER now correctly labeled as "Timer" (was "Multistate")
• OTHER now correctly labeled as "Multistate" (was "Timer")
• backend/proteins/models/protein.py:134-135

✅ Help text typo fixed (commit f7c2a98)

• Changed "does not fluorescence" → "does not fluoresce"
• backend/proteins/models/fluorescence_data.py:87

✅ ex_band/em_band return types improved (commit a77e36c)

• Changed from tuple[float, float] | Literal[False] to cleaner tuple[float, float] | None
• Methods now safely check if spectrum exists before calling .width()
• backend/proteins/models/fluorophore.py:243-251

✅ State.save() EntityType reference fixed

• Now correctly uses Fluorophore.EntityTypes.PROTEIN
• DyeState also correctly uses Fluorophore.EntityTypes.DYE
• backend/proteins/models/protein.py:593
• backend/proteins/models/dye.py:80

✅ wave_to_hex inlined in migration (commit 918bc96)

• Function is now defined within the migration file for deterministic behavior
• Migration no longer depends on live codebase

✅ Migration comments clarified (commit 6bd7a88)

• Removed misleading comment about "will be populated by rebuild_attributes()"
• Migration logic is now clearer

3. Comprehensive Test Coverage Added

• New file: backend/tests/test_proteins/test_fluorophore.py (423 lines, 20 tests)
• Test classes cover:
  • TestRebuildAttributesBasicWaterfall - waterfall logic
  • TestRebuildAttributesTrustedPriority - trusted measurements
  • TestRebuildAttributesPrimaryReferencePriority - primary ref priority
  • TestRebuildAttributesPriorityOrder - full priority stack
  • TestRebuildAttributesPinnedFields - admin overrides
  • TestRebuildAttributesNoMeasurements - edge case
  • TestRebuildAttributesAutoTrigger - save/delete hooks
  • TestGetPrimaryReferenceId - helper method tests

---

Current Assessment

✅ Strengths (Unchanged from Previous Review)

Schema Design:

• Clean MTI architecture with proper separation of concerns
• Excellent caching strategy with source_map provenance tracking
• Extensible design for future fluorophore types

Migration Quality:

• Comprehensive data preservation
• Performance-optimized bulk updates
• Proper handling of django-reversion history

API Compatibility:

• REST API backward compatible
• GraphQL properly handles MTI polymorphism
• No breaking changes detected

🟡 Minor Items Remaining

1. Validation Not Added
As noted in the previous review, there's still no validation to prevent:

• Creating measurements without a reference when one should be required
• Marking multiple measurements as "trusted" for the same field
• Having Fluorophore cached values that don't match any measurement

Impact: Low - These are edge cases that are unlikely to occur in normal usage. The auto-rebuild logic on measurement save/delete means orphaned cache data won't persist. Multiple trusted measurements will use the most recent one, which is reasonable fallback behavior.

Recommendation: Optional - Add validation in a follow-up PR if these become issues in practice.

2. N+1 Query Risk in rebuild_attributes()
The method uses self.measurements.all() and then sorts in Python rather than in the database. For fluorophores with many measurements, this could be inefficient.

Impact: Very Low - Most fluorophores have <5 measurements. The code is clean and readable. Premature optimization isn't warranted.

Recommendation: Monitor in production; optimize only if it becomes a bottleneck.

3. Minor Type Hint Opportunity
Fluorophore.as_subclass() return type is Self, losing specific subclass type info. Could use @overload for better IDE support.

Impact: Negligible - Type hints work fine for practical use cases.

4. SpectrumOwnerUnion Incomplete
GraphQL union only includes State:

class SpectrumOwnerUnion(graphene.Union):
    class Meta:
        types = (State,)  # Missing DyeState, Camera, Filter, Light?

Impact: Unknown - May be legacy code or intentionally limited. Worth verifying if this union is used.

Recommendation: Check if this union is used in any GraphQL queries. If unused, consider removing it. If used, verify the limited type set is intentional.

---

Copilot Review Comments Status

Reviewed all 22 Copilot comments:

Addressed:

• ✅ ex_band/em_band AttributeError fix (lines 243-251 now safe)
• ✅ SwitchingChoices label swap (TIMER/OTHER fixed)
• ✅ "does not fluorescence" typo (fixed to "fluoresce")
• ✅ State.save() EntityType reference (now uses Fluorophore.EntityTypes.PROTEIN)

Not Addressed (by design or false positives):

• ⚠️ save() rebuild_cache kwarg - This is intentional; Django allows custom kwargs
• ⚠️ Unused imports - Many are TYPE_CHECKING imports needed for type hints
• ⚠️ rebuild_attributes() recursion - Not an actual issue; uses update_fields implicitly

Needs Verification:

• ❓ Various unused imports flagged by Copilot - Most are false positives (TYPE_CHECKING imports)

---

Final Recommendations

Before Merging

Required:

• ✅ All critical issues have been addressed

Manual Testing Checklist (as originally noted):

1. Admin Inline for State - Test add/edit/delete State through Protein admin
2. Spectrum submission form - Submit new dye and protein spectra
3. Microscope efficiency reports - Run full report with mixed proteins/dyes
4. GraphQL queries - Test owner field resolution and verify SpectrumOwnerUnion usage
5. Celery task calc_fret() - Verify FRET calculations work
6. Reversion - Test reverting old and new State/Dye revisions

Optional Follow-ups

These can be addressed post-merge if needed:

• Add measurement validation if edge cases occur in practice
• Verify SpectrumOwnerUnion usage and expand or remove as needed
• Optimize rebuild_attributes() if performance monitoring shows it's needed
• Clean up any remaining unused imports (most Copilot flags are false positives)

---

Final Verdict

Status: ✅ READY TO MERGE

Confidence Level: 95% (up from 85% in previous review)

Rationale:

• All critical issues identified in the first review have been fixed
• Primary reference sorting logic is now complete and well-tested
• Comprehensive test suite added (20 tests covering all priority paths)
• All Copilot bugs have been addressed or verified as false positives
• Migration is production-ready with deterministic behavior
• API compatibility maintained
• The minor items remaining are truly optional enhancements

The only blocker is manual testing - the code review is complete and the implementation is solid. Once the manual testing checklist is verified (particularly admin inlines, spectrum forms, and efficiency reports), this is ready for production deployment.

---

Excellent work addressing all the feedback! The refactored rebuild_attributes() method is particularly well-designed, and the test coverage is comprehensive. The schema migration is production-ready.

---