♻️ refactor: share LocalFlowOrderer's chord attachment across both dispatches

[nstarman]

The recurring bug

LocalFlowOrderer.order's Quantity dispatch (in _interop/interop_unxt.py) reaches algorithm._local_flow_walk directly, rather than delegating to the plain dispatch the way MSTOrderer/SOMOrderer do. That keeps it genuinely unit-aware via quax.quaxify, but it means any postprocessing step added to the plain path has to be separately re-added to the Quantity path, with nothing to catch a forgotten copy.

Three fields have drifted this way in turn:

added to the plain path	unit-ful behaviour before the second fix
velocity_aware (#56)	always False — a chained SOMOrderer silently dropped to position-only
chord (#67)	always None — the encoder silently used its old inline fallback
start_idx from init (#70)	None reached the walk as a start index — 13 tests failed

Only the third failed loudly.

What's already unified, and what wasn't

velocity_aware and start_idx turned out to already be fixed on main: the former is derived once inside _local_flow_walk itself from the shared config (so both paths get it automatically), and the latter already goes through the shared _resolve_start_idx helper. chord was the one piece still duplicated — _with_chord in the plain dispatch, a second inline chord_along_ordering call in the Quantity dispatch.

The fix (option 1 + 3 from the issue)

Renamed _with_chord to _finalize and gave it a positions parameter instead of always reading result.positions, so both dispatches can call the same function — the plain one directly, the Quantity one on unit-stripped positions before reattaching the unit. A postprocessing step landing only in one of the two call sites is exactly the drift this closes off.

Paired with test_localflow_quantity_agrees_with_plain_field_by_field, which compares every field of the result generically via dataclasses.fields rather than naming a fixed set — so a future field missing from the Quantity path fails this test instead of shipping silently, which is the actual acceptance criterion in #71. Verified it fails when the _finalize call is reverted (confirmed: crashes trying to unit-convert chord=None) and passes with it restored.

Full test suite: 1006 passed, 1 skipped.

Closes #71.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟢 Approval recommended

The refactor unifies previously duplicated logic with a strong regression test to prevent recurrence, and no correctness or API issues were found in the reviewed changes.

Review effort: Lite
Findings: 1

Open (1)

• Assert MST init indices match before comparing dispatch behavior · New

What changed in this PR

Refactors LocalFlowOrderer’s post-processing so both the plain-array and unxt.Quantity dispatches share the same chord-attachment logic, preventing future drift between the two execution paths in phasecurvefit’s ordering pipeline.

Changes:

• Replaces the plain-path-only _with_chord helper with a shared _finalize(result, positions) function.
• Updates the Quantity dispatch to call _finalize on unit-stripped positions, then reattach units to the resulting chord.
• Adds a regression test that compares all result fields between plain and Quantity runs via dataclasses.fields, to catch any future divergence automatically.

File	Description
tests/​unit/​test_orderers_unxt.py	Adds a generic field-by-field parity test between plain vs Quantity LocalFlow results to prevent silent drift.
src/​phasecurvefit/​_src/​orderers/​localflow.py	Introduces _finalize(result, positions) and uses it in the plain dispatch to centralize post-processing.
src/​phasecurvefit/​_interop/​interop_unxt.py	Switches LocalFlow’s Quantity dispatch to reuse _finalize and then unit-reattach the chord.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@2385f16). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #137   +/-   ##
=======================================
  Coverage        ?   91.97%           
=======================================
  Files           ?       34           
  Lines           ?     1982           
  Branches        ?      109           
=======================================
  Hits            ?     1823           
  Misses          ?      129           
  Partials        ?       30           

☔ View full report in Codecov by Harness.
📢 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.

[Copilot]

Copilot review overview

🟡 Changes recommended

The newly added generic field-by-field test is currently brittle for legitimate NaNs and future tuple/list fields, which can cause false failures even when both dispatches agree.

Review effort: Lite
Findings: 1

Open (1)

• Allow NaNs in chord ordering comparison · New

Resolved since last review (1)

• Assert MST init indices match before comparing dispatch behavior

Previously missed (1)

In code that hasn't changed since last review

Recursively strip tuple/list fields for comparison

tests/​unit/​test_orderers_unxt.py:145

_strip_for_comparison claims to handle tuple/list-shaped fields, but it currently returns tuples/lists unchanged. If a future result field is a tuple/list containing arrays, the final assert plain_val == q_val will raise due to ambiguous truth value from array element comparisons. Strip tuple/list contents recursively to keep the test future-proof.

[Copilot]

Copilot review overview

🟢 Approval recommended

The refactor unifies chord finalization across dispatches and the new generic field-by-field test provides a strong guard against future drift.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Allow NaNs in chord ordering comparison