Repository navigation
Root the indexed tree before computing Faith's PD (Codex P1 on #234) - #238
Conversation
Codex (P1) and CodeRabbit (major) both flagged this on #234: IQ-TREE infers an *unrooted* tree for a reversible model, so the root of a `reference.treefile` is only whichever node happened to be serialized first. `analyse_phylogeny` used that root directly, while the legacy `faith_pd(counts, taxa, tree)` path midpoint-roots first via `_ensure_rooted_tree`. UniFrac is unaffected — it sums edge partitions and re-rooting preserves them — but Faith's PD is a path-to-root quantity, so for a sample with a single active tip the indexed path reported 1.0 where the legacy path reported 1.5 for the same tree, and an equivalent rerooted serialization could give a different number again. `ensure_rooted` now applies the legacy convention to `PhylogeneticTree`: a root with more than two children is midpoint-rooted by `midpoint_root` (the implementation already verified against scikit-bio reference outputs) and an already bifurcating tree is returned untouched. To reuse that audited code without reimplementing it, `_to_phylo`/`_from_phylo` convert to `PhyloNode` and back; the round trip preserves labels, tip names, lengths and the parents-precede-children ordering the roll-up relies on. `analyse_phylogeny`, `faith_pd(tree, counts, tip_names)` and `unifrac(tree, counts, tip_names)` root before rolling up; the roll-up overloads take a matrix, so they document that it must come from an already rooted tree. The cladistics `results.json` now records the convention in a `rooting` field, mirrored as an optional `CladisticsResult.rooting` in the frontend types. Tests pin the convention: an unrooted trifurcating tree roots to two children, PD for a lone tip matches the legacy `faith_pd` path exactly (1.5, versus the 1.0 the raw serialization gives), and a rooted and an unrooted serialization of the same inferred tree agree on `faith_pd`, both UniFrac matrices and `total_branch_length`. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 33 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Fork governance
🟣 critical · commit
Hypatia scan: 141 finding(s)
panic-attack assail: 17 finding(s)
pons asinorum: 8 finding(s)
rhodibot (RSR compliance): not_run — enrolled, but no report-back was received
panicbot (panic recovery, error handling): not_run — enrolled, but no report-back was received
echidnabot (formal verification: proofs/): not_run — enrolled, but no report-back was received
seambot (integration health: API routes): not_run — enrolled, but no report-back was received
finishingbot (release readiness): not_run — enrolled, but no report-back was received
accessibilitybot (WCAG on the React UI): not_run — enrolled, but no report-back was received
glambot (presentation quality): not_run — enrolled, but no report-back was received
cipherbot: not_applicable — no cryptography is implemented in this repository
sustainabot: not_applicable — fleet adapter slot reserved; no adapter exists yet (gitbot-fleet README)
Scope: static findings reported by each row's tool at the named commit. Purple, red, yellow and blue grade the worst finding present; orange is evidence missing, malformed or a scanner that did not run; grey is an explicit scope exclusion with its reason. No row is a security certification. report.json, badge.json and the raw findings are in the verdict-grid artifact on the linked run. |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a92c06569
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # IQ-TREE infers an unrooted tree for a reversible model, so PD is only | ||
| # meaningful relative to a stated rooting convention; record it next to the | ||
| # other metric definitions rather than leaving it implicit. | ||
| result["rooting"] = "midpoint (unrooted trees; Faith PD is root-dependent, UniFrac is not)" |
There was a problem hiding this comment.
When consumers interpret the stored provenance for weighted_unifrac, this statement is incorrect: the indexed weighted metric is normalized by sum(branch * (a+b)), and rerooting complements descendant proportions on affected edges, which can change that denominator even though the numerator and unweighted UniFrac remain invariant. Restrict the invariance claim to unweighted/raw weighted UniFrac or state that the normalized weighted result also follows the recorded midpoint-rooting convention.
Useful? React with 👍 / 👎.
| function ensure_rooted(tree::PhylogeneticTree)::PhylogeneticTree | ||
| length(tree.nodes[tree.root].children) <= 2 && return tree |
There was a problem hiding this comment.
For a genuinely rooted multifurcating input such as [&R](A:1,B:2,C:3);, the indexed parser discards the rooting comment and this condition treats the three-child root as unrooted, silently midpoint-rerooting it and changing Faith's PD and the reported clades. Root arity is not a rootedness indicator—a degree-two serialization can likewise represent an unrooted tree—so rootedness needs to be retained explicitly or supplied by the caller rather than inferred here.
Useful? React with 👍 / 👎.
| PhyloNode("n$id", node.name, is_root ? 0.0 : something(node.length, 0.0), | ||
| nothing, nothing, children) |
There was a problem hiding this comment.
When midpoint rooting reverses an edge carrying an IQ-TREE internal support label, this conversion stores that label as the vertex name while passing nothing as the edge support. midpoint_root therefore leaves the label attached to the original vertex instead of moving it with the supported bipartition, and _from_phylo can emit the support value on a different clade in the cumulative tree. Parse internal support labels into edge metadata and restore them on the corresponding rerooted edge.
Useful? React with 👍 / 👎.
| """ | ||
| function ensure_rooted(tree::PhylogeneticTree)::PhylogeneticTree | ||
| length(tree.nodes[tree.root].children) <= 2 && return tree | ||
| rooted, changed = midpoint_root(_to_phylo(tree)) |
There was a problem hiding this comment.
When an unrooted tree has zero length on every edge—a valid parser input and a possible result when trimming leaves identical sequences—this call reaches midpoint_root, whose strict farthest-tip comparisons select the same tip as both diameter endpoints and then index path[2], throwing a BoundsError. The indexed analysis previously accepted zero branch lengths and produced zero-valued metrics, so handle a zero diameter explicitly rather than failing the whole cladistics build.
Useful? React with 👍 / 👎.
…green Julia tests on main) (#240) `main` is red on `Julia tests` — 5727 pass, **1 fail**, 0 error — and it was already red when #239 merged: that PR's own `Julia tests` check failed on the same line. ``` Dual-hierarchy CladeCumulus, polyphyly detection, drag-and-drop, and HTTP routes: Test Failed at test/unit/test_clade_cumulus.jl:730 Expression: ≈((jp_body.diversity.weighted_unifrac[1])[2], 0.15636363636363637, atol = 1.0e-9) Evaluated: 0.1378181818181818 ≈ 0.15636363636363637 ``` ## Which number is right The expectation predates #239's own fix, which its description states as "`totals`/`total_a`/`total_b` library-size denominators". The route now divides by each sample's 55-read library instead of the 34 and 20 reads that happened to reach the tree, which is the whole point of that change — otherwise the unplaced mass is renormalised back onto the tips. Working the fixture out from that definition, for the grafted tree of `((Ref_1:0.10{0},Ref_2:0.20{1})0.96:0.30{2},Ref_3:0.40{3}){4};` with ASV_1 placed at LWR 0.60 on edge 0 (`distal 0.05`, `pendant 0.02`) and ASV_2 at LWR 1.00 on edge 1: | edge | length | SampleA (24 / 10 placed) | SampleB (20 placed) | | --- | --- | --- | --- | | ASV_1 pendant | 0.02 | 24/55 | 0 | | Ref_1 remainder | 0.05 | 24/55 | 0 | | ASV_2 pendant | 0.02 | 10/55 | 20/55 | | Ref_2 remainder | 0.15 | 10/55 | 20/55 | | joining clade | 0.30 | 34/55 | 20/55 | | Ref_3 | 0.40 | 0 | 0 | ``` raw weighted UniFrac = (0.02*24 + 0.05*24 + 0.02*10 + 0.15*10 + 0.30*(34-20)) / 55 = 7.58 / 55 = 0.1378181818... ``` That is exactly what the route returns, so the code agrees with its documented definition and the constant is stale. Nothing else in the block changes: `mapped_tips == 2`, `unplaced_reads` 21/35 and `root.cumulative_count ≈ 110` all still hold, and no other test in the suite fails. ## Change One test assertion. It now spells out the arithmetic — `≈ 7.58 / 55` with the edge-length derivation in a comment — so a future convention change has to argue with the definition rather than with a bare decimal. No production code is touched. ## Verified The full suite on this branch is CI; locally, tree-sitter parses the edited statement and `git diff` shows only the assertion. Everything else on `main` (including #238's rooting tests) passed in the run that caught this. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
Closes the last substantive finding on #234.
The finding
Codex P1 — "Root the IQ-TREE output before computing rooted metrics" — and CodeRabbit major — "Define and enforce one rooting convention for Faith's PD" — both flagged the same thing, and both are right.
step_commandsruns IQ-TREE with a reversible substitution model, which infers an unrooted tree, so the root ofreference.treefileis only whichever node IQ-TREE happened to serialize first.analyse_phylogenyused that node directly, while the legacyfaith_pd(counts, taxa, tree)path midpoint-roots first (_ensure_rooted_tree). UniFrac is unaffected — it sums edge partitions, and re-rooting preserves them — but Faith's PD is a path-to-root quantity:an equivalent rerooted serialization of the same inferred tree could give a different number again.
The fix
New
ensure_rooted(::PhylogeneticTree), using exactly the legacy convention: a root with more than two children is midpoint-rooted by the existingmidpoint_root, and an already bifurcating tree is returned untouched (identical object). To avoid reimplementing an algorithm that is already verified against scikit-bio reference outputs,_to_phylo/_from_phyloconvert toPhyloNodeand back; the round trip preserves tip names, internal labels, branch lengths and the parents-precede-children ordering the roll-up depends on.analyse_phylogeny,faith_pd(tree, counts, tip_names)andunifrac(tree, counts, tip_names)root before rolling up, so the pipeline and the legacy API can no longer disagree.faith_pd(tree, cumulative),unifrac(tree, cumulative)) take a matrix and cannot root it for you; their docstring now says the roll-up must come from an already rooted tree.results.jsonrecords the convention in arootingfield alongsideweighted_unifrac_definition, mirrored as optionalCladisticsResult.rootingin the frontend types.docs/cladistics/clade-cumulus.mddocuments the convention and why the two metrics differ.Tests
1.5— and equal tofaith_pd(counts, taxa, tree)on the same Newick string, which is the parity the finding asked for; the unrooted roll-up is asserted to be the wrong1.0.faith_pd,unweighted_unifrac,weighted_unifracandtotal_branch_length(6.0).The only other #234 finding, CodeRabbit's docstring nit on
weighted=truebeing normalised weighted UniFrac, is already onmain(it landed in833c420).