✨ feat: chainable orderers, and let the MST pick the walk's start point - #70
Merged
Merged
Conversation
github-actions
Bot
added
📝 Documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
=======================================
Coverage ? 91.22%
=======================================
Files ? 31
Lines ? 1596
Branches ? 74
=======================================
Hits ? 1456
Misses ? 115
Partials ? 25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
_resolve_start_idx currently materializes all visited indices even though only the first is needed, creating avoidable device/host overhead in the new default-start resolution path.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
This PR introduces sequential composition of orderers via a new ChainOrderer (and the a | b operator), threading prior stage results forward as init, and updates LocalFlowOrderer to optionally take its walk start point from the prior stage’s ordering when start_idx is unset.
Changes:
- Add
ChainOrdererplusAbstractOrderer.__or__, and threadinitthroughpcf.order(...)and stageorder()calls while preserving legacy head-stage compatibility. - Update
LocalFlowOrderersostart_idx=Noneresolves frominit(else falls back to0), including theunxtQuantity path. - Add tests and documentation covering chaining semantics and the “MST picks the walk start” behavior.
| File | Description |
|---|---|
| tests/test_orderers_unxt.py | Adds a regression test ensuring the Quantity LocalFlow path resolves start_idx from init consistently. |
| tests/test_chain.py | New test suite for chaining behavior, legacy compatibility, and start_idx resolution from prior results. |
| src/phasecurvefit/orderers.py | Exposes ChainOrderer in the public orderers API and updates module docs. |
| src/phasecurvefit/_src/orderers/mst.py | Accepts (and ignores) init for chainability; centralizes component-key validation. |
| src/phasecurvefit/_src/orderers/localflow.py | Adds _resolve_start_idx, changes default start_idx to None, and threads init into start selection. |
| src/phasecurvefit/_src/orderers/chain.py | Implements ChainOrderer flattening and stage-to-stage init threading. |
| src/phasecurvefit/_src/orderers/base.py | Adds _check_component_keys, extends the abstract order() contract with init, adds __or__, and updates pcf.order facade. |
| src/phasecurvefit/_interop/interop_unxt.py | Extends Quantity dispatches to accept/forward init and resolves start_idx consistently; makes _require_usys tolerate metadata=None. |
| docs/guides/orderers.md | Documents chaining and the new default start-point behavior when chained after MST. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new chaining docs and Quantity-orderer type signatures contain correctness/clarity issues (stored as review comments) that should be fixed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
Resolved since last review (1)
nstarman
force-pushed
the
claude/chain-orderer
branch
from
September 19, 2026 21:26
99d6477 to
a632605
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new chaining documentation/docstrings reference a non-existent SOMOrderer and overstate chainability in a way that contradicts the legacy-orderer behavior exercised by the new tests.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (3)
Resolved since last review (2)
Orderers compose with `|`. Each stage receives the previous stage's result as `init`; a stage that can use a prior ordering does, and the rest accept and ignore it, so every orderer is chainable. `ChainOrderer.order` is a plain method rather than a `plum` dispatch: it forwards its arguments untouched so each stage's own dispatch -- including the `unxt` Quantity ones -- selects itself. `|` flattens, so `a | b | c` is one three-stage chain rather than a nest. Back-compatible: with no prior result the chain omits `init` entirely, so an orderer written against v0.3.1's `order()` still works as the head of a chain. `_require_usys` now accepts `None`, since both the `order` facade and `ChainOrderer` forward `metadata` unconditionally. The two Quantity `order` dispatches are typed to match -- `StateMetadata | None = None` rather than a non-optional annotation over a shared `StateMetadata()` default instance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nstarman
force-pushed
the
claude/chain-orderer
branch
from
September 19, 2026 21:43
a632605 to
9704602
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The chaining and start-point seeding behavior is implemented consistently across plain and Quantity paths with strong test coverage, and the remaining feedback is a non-blocking performance refinement.
Review effort: Lite
Findings: 1
Open (1)
nstarman
force-pushed
the
claude/chain-orderer
branch
from
September 19, 2026 23:45
9704602 to
6c83b20
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The chaining API and start_idx resolution changes are consistent across plain and Quantity paths, preserve legacy behavior by omitting init when absent, and are covered by focused new tests.
Review effort: Lite
Findings: None
Resolved since last review (1)
`LocalFlowOrderer` needs to begin at an end of the curve, and `start_idx` made
the caller name that index by hand -- which means knowing the answer before
having ordered anything. `start_idx=0` is only right when the input happens to
arrive already ordered, which is what an orderer exists to fix.
An `MSTOrderer` has no such problem: it orders along the graph diameter, tip to
tip, so its first observation is an endpoint. `start_idx` now defaults to
`None`, meaning "take it from `init`, else 0", so
pcf.orderers.MSTOrderer(k=16, jump_cap=3.0) | pcf.orderers.LocalFlowOrderer()
starts the walk at a real tip. On a shuffled 400-point arc that moves |rho|
from 0.68 to 1.00.
An explicit `start_idx` still wins, chained or not, so nothing changes for
callers who already pass one, and an unchained walk keeps starting at 0.
The index is found with `argmax` over the validity mask rather than
`indices[indices >= 0]`, which would materialize a variable-length array and
pull it to host just to read element 0. The Quantity dispatch resolves it too:
it reaches `_local_flow_walk` directly, and without that `None` arrives as a
start index.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two epitrochoid walk tutorials picked their start index out of ground
truth:
start_idx = int(np.argsort(order)[0])
`order` is the shuffle permutation, so this finds where the *true* first point
landed -- information a reader analysing real data does not have. The comment
called it "the point closest to the starting point", which rather hid that.
Both now chain a velocity-aware MSTOrderer into the walk and let it supply the
index. On the epitrochoid the MST independently nominates the same observation
the ground-truth lookup did (index 1839), so the ordering is unchanged at
|rho| = 1.000 over 2044 points -- the tutorial simply stops cheating.
The severing matters: a plain MSTOrderer short-circuits across the crossings
and nominates a bad tip (index 35, |rho| = 0.35). The narrative says so.
`start_idx` is re-derived after the walk as `walkresult.ordering[0]` for the
plots that mark the start, which is the index the chain actually used.
The two stream walk tutorials are deliberately left alone: they start at the
observation nearest the progenitor and walk `direction="both"` outward along
both arms. That start is physically motivated rather than ground truth, and an
MST *tip* would be the wrong point for it.
The orderers guide gains a chaining section. It says plainly that chaining does
*not* narrow the data -- every stage is handed the full positions and
velocities, and `init` is context a stage may ignore -- so an upstream stage's
rejections are inherited only by a stage written to restrict itself.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nstarman
force-pushed
the
claude/chain-orderer
branch
from
September 20, 2026 02:50
6c83b20 to
c1f7fb5
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The API changes are internally consistent, preserve legacy head-stage compatibility by omitting init= when absent, and are backed by targeted tests and updated documentation (including the Quantity dispatch path).
Review effort: Lite
Findings: None
Resolving `start_idx` from `init` ended in `int(...)`. Under `jit` the prior
stage's `indices` are traced, so a chain raised ConcretizationTypeError while a
single orderer compiled fine -- the chaining API was quietly incompatible with
the transform story the package sells.
The index is now left as a JAX scalar. That in turn exposed the walk's bounds
check, whose comment claimed to "use plain Python check if not traced" while
doing an unguarded `if`: it raised on a traced index before it could report
anything. A concrete index is still checked in Python, so the error arrives
immediately as before; a traced one is checked with `eqx.error_if`.
transform single orderer chain (before -> after)
grad ok ok -> ok
jit ok raises -> ok
vmap ok raises -> ok
Covered by tests, including that the bounds check still fires.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nstarman
added a commit
that referenced
this pull request
Sep 24, 2026
`.coverage` is a coverage.py SQLite database regenerated by every `pytest --cov` run. It was committed by accident in #70 and has been churning ever since. Untrack it and ignore it going forward, along with the parallel-run shards (`.coverage.*`) and the HTML report directory (`htmlcov/`). No tooling references the path: `pyproject.toml` only has the `[tool.coverage]` config table, and CI uploads `coverage.xml` (via `--cov-report=xml`) to Codecov rather than the database itself. History is left alone — this only stops future commits from carrying it. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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.
Split out of #56 so the chaining machinery can be reviewed on its own, and because it immediately fixes a real papercut in
LocalFlowOrderer.Chaining
Orderers compose with
|. Each stage receives the previous stage's result asinit; stages that can use a prior ordering do, and the rest accept and ignore it, so every orderer is chainable.|builds aChainOrdererand flattens, soa | b | cis one three-stage chain.order()on the chain is a plain method rather than aplumdispatch — it forwards arguments untouched so each stage's own dispatch (including theunxtQuantity ones) selects itself.Back-compatible: when there is no prior result the chain omits
initentirely, so an orderer written against v0.3.1'sorder()still works as the head of a chain. There is a test for that.The walk's start point
LocalFlowOrdererhas to begin at an end of the curve, andstart_idxmade the caller name that index by hand — which means knowing the answer before having ordered anything. The defaultstart_idx=0is only correct when the input happens to arrive already ordered, which is exactly what an orderer exists to fix.An
MSTOrdererhas no such problem: it orders along the graph diameter, tip to tip, so its first observation is an endpoint.start_idxnow defaults toNone, meaning "take it frominit, else 0":LocalFlowOrderer()alone (falls back to index 0)MSTOrderer(...) | LocalFlowOrderer()An explicit
start_idxstill wins, chained or not, so nothing changes for callers who pass one; an unchained walk still starts at 0.Note for reviewers
The Quantity dispatch in
interop_unxtreaches_local_flow_walkdirectly rather than going through the plainordermethod, so it needed the same resolution — without itNonearrived as a start index and 13 tests failed. Both paths are covered.Test plan
start_idxwins; an unchained walk keeps starting at 0; an empty prior ordering falls back; and the Quantity path resolves identically.prek run --all-filesand ruff clean.🤖 Generated with Claude Code