Repository navigation
Conversation
Initial version of the ARPIP algo with homolgy path recpntruction
Integration test for PIP ASR using data from the ARPIP paper. Checks that homology paths are reconstructed correctly.
Added function to transform Mapping to FixedBitSet to alignment mod
Fixed test scenarios for ARPIP as they were checking the wrong tree node ids
Refactored PIP ASR to separate struct to match e.g. `ParsimonyPresenceAbsence`
Refactor to have `AncestralSequenceReconstruction` trait impl first for PIP asr and helpers second
Rename deletion_probabilities_at_nodes -> deletion_probabilities
Correct implementation of ARPIP indel inference with proper insertion probability computation
Added tests for when the most likely insertion location neitger at first node (MRCA) or root
Changed `Alignment::alphabet()` to return static ref to the alphabet to allow creating an MASA from an MSA.
Split `validate_ids_with_ancestors` in two methods, `validate_taxa_ids` and `validate_ancestor_ids` to allow validating them independently.
Added method to create an MASA from an Alignment and provided ancestral sequences in a `Sequence` object (`from_msa_with_ancestral`). Added a function `warn_on_extinct_columns` to reduce duplication for the check.
Use `warn_on_extinct_columns` in AncestralAlignment creation from all sequences
Added tests for `MASA::from_msa_with_ancestral` including correct creation and different types of possible errors
Cleaned up the tests for pip asr intermediate (tmp) values
Use `from_msa_with_ancestral` in PIP ASR to reuse MSA sequences rather than recreate a full `Sequences` object and use `from_aligned_with_ancestral_unchecked`.
Split `from_msa_with_ancestral` into two methods, `from_msa_with_ancestral` and `from_msa_with_ancestral_unchecked`. `from_msa_with_ancestral` has a default impl with the necessary checks and calls `from_msa_with_ancestral_unchecked`.
Add doctests for `from_msa_with_ancestral` and `from_msa_with_ancestral_unchecked`
Rename `set_internal` to `compute_internal_probabilities` and `set_leaf` to `compute_leaf_probabilities` to distinguish between the upward pass and the downward pass through the tree.
Refactored internal character presence determination into separate method and include presence in the tmp storage
Refactor to extract deciding deletion location (node or both children) to a separate method
Test Results1 184 tests +55 1 183 ✅ +55 15m 35s ⏱️ - 12m 16s Results for commit 5eaf7e0. ± Comparison against base commit 71d7ce2. This pull request removes 8 and adds 63 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical defects can select incorrect ancestors, require quadratic memory, and admit invalid or ambiguous sequence IDs that distort alignment statistics and ancestral updates.
5 open issues
Reject duplicate tree-node IDs before ancestral-record validation · New Preserve strict leaf-only validation for mixed ancestor records · New Store local deletion flags instead of propagated descendant sets · New Use log-space insertion scores to prevent precision loss and underflow · New Compute the tree leaf-ID set once during validation · New
What changed in this PR
Adds PIP-based ancestral indel reconstruction to the existing ancestral sequence reconstruction interface, with alignment-construction APIs and ARPIP comparison tests.
Changes:
- Introduces
PIPIndelsto infer ancestral character presence from insertion and deletion probabilities. - Adds checked and unchecked ancestral-alignment constructors, refactors ID validation, and standardizes static alphabet references.
- Adds reconstruction tests and ARPIP reference data.
| File | Description |
|---|---|
phyne/tests/pip_asr.rs |
Compares reconstructed ancestral mappings with ARPIP results. |
phyne/src/pip_model/tests.rs |
Adds a reconstruction integration test. |
phyne/src/pip_model/mod.rs |
Exports the ASR module and PIPIndels. |
phyne/src/pip_model/asr.rs |
Implements indel reconstruction and focused unit tests. |
phyne/src/phylo_info/phyloinfo_builder.rs |
Refactors leaf and ancestor ID validation with tests. |
phyne/src/alignment/tests.rs |
Tests mapping conversion and ancestral-alignment constructors. |
phyne/src/alignment/mod.rs |
Adds constructors and mapping utilities; refactors alphabet access and extinct-column warnings. |
phyne/data/pip/arpip/sim-0_new.newick |
Supplies the ARPIP comparison tree. |
phyne/data/pip/arpip/sim-0_msa_new.fasta |
Supplies the observed leaf alignment. |
phyne/data/pip/arpip/sim-0_inferred.fasta |
Supplies reference ancestral and leaf sequences. |
Review effort: Balanced
| "ancestral sequences must have the same length as the provided alignment" | ||
| ); | ||
| } | ||
| validate_ancestor_ids(&phylo.tree, &ancestral_seqs)?; |
There was a problem hiding this comment.
copilot added
phylo.tree.node_ids_are_unique()?;
This seems like a valid suggestion to me
There was a problem hiding this comment.
Like with the validate_taxa_ids it think this sequences object could contain seqs with ids that are in the tree tips set. they would not be detected by validate_ancestor_ids, in from_msa_with_ancestral they are silently ignored. So not quite like the MSA::from_aligned which added the additional seqs to the .seqs anyway rather than silently ignoring it.
| let tip_sequence_ids = HashSet::from_iter(sequences.into_iter().filter_map(|rec| { | ||
| let id = rec.id().to_string(); | ||
| if !internal_ids.contains(&id) { | ||
| Some(id) | ||
| } else { |
There was a problem hiding this comment.
This is true:
let tree = tree!("((A2:1,B3:1)I1:1,(C5:1,D6:1)I4:1)I0:0;");
let sequences = Sequences::with_alphabet_unchecked(
vec![
record_wo_desc!("A2", b"XXX--X"),
record_wo_desc!("B3", b"X-X--X"),
record_wo_desc!("C5", b"X---X-"),
record_wo_desc!("D6", b"--XXX-"),
record_wo_desc!("I4", b"--XXX-"),
],
Alphabet::dna(),
);
let msa = MSA::from_aligned(sequences, tree.clone()).unwrap();
for seq in msa.msa.seqs() {
println!("{}: {}", seq.id(), String::from_utf8_lossy(seq.seq()));
}
for map in msa.msa.leaf_maps().values() {
println!("leaf map: {map:?}");
}prints
A2: XXXX
B3: XXX
C5: XX
D6: XXX
I4: XXX
leaf map: [Some(0), None, None, None, Some(1), None]
leaf map: [Some(0), Some(1), Some(2), None, None, Some(3)]
leaf map: [Some(0), None, Some(1), None, None, Some(2)]
leaf map: [None, None, Some(0), Some(1), Some(2), None]
There was a problem hiding this comment.
Either add a comment that set sizes should still be check or go back to 3 separate methods:
- one that checks seq ids = tree tip ids
- one that checks seq ids = tree internal ids
- one that check seqs ids = all tree ids
This way there might be some duplicate code, but its more clear what each method does and when to apply it and it would catch incorrect seqs set sizes automatically.
| tmp.f[idx] = f_child1.clone().component_mul(&tmp.f[children[1]]); | ||
| tmp.f[idx] *= 1.0 - tmp.deletion_probs[idx]; | ||
| tmp.p[idx] = | ||
| f_child1.component_mul(&tmp.f[children[1]]) * tmp.insert_and_survive_probs[idx]; |
| /// Tree is consumed and stored together with the ancestral alignment, because the alignment is only | ||
| /// meaningful in the context of the tree that it was built from. |
There was a problem hiding this comment.
I think this comment could be reworked to fit better: Its not the tree that is consumed, but the PhyloInfo.
| /// Tree is consumed and stored together with the ancestral alignment, because the alignment is only | |
| /// meaningful in the context of the tree that it was built from. | |
| /// PhyloInfo (i.e., Tree and MSA) is consumed, the ancestral alignment and corresponding tree are returned together, | |
| ///because the alignment is only meaningful in the context of the tree that it was built from. |
There was a problem hiding this comment.
Yes, definitely. I think I copied the initial comment from from_aligned_with_ancestral_unchecked, which only gets a tree. I will rewrite, but now I am also thinking if I should flip the order of arguments, since from_aligned_with_ancestral_unchecked takes all_seqs and tree, whereas from_msa_with_ancestral_unchecked takes a PhyloInfo and ancestral_seqs.
Or, potentially, should we flip the arguments for from_aligned_with_ancestral_unchecked? It makes more sense to have tree+msa first and sequences second for the from_msa_... method, but then it clashes with the sequences+tree for the other one in my mind. It did not matter while there was only one method, but now it bothers me...
There was a problem hiding this comment.
i guess either
tree, seqs & phyloInfo, seqs
or
seqs, tree & seqs, PhyloInfo
| let tip_sequence_ids = HashSet::from_iter(sequences.into_iter().filter_map(|rec| { | ||
| let id = rec.id().to_string(); | ||
| if !internal_ids.contains(&id) { | ||
| Some(id) | ||
| } else { |
There was a problem hiding this comment.
This is true:
let tree = tree!("((A2:1,B3:1)I1:1,(C5:1,D6:1)I4:1)I0:0;");
let sequences = Sequences::with_alphabet_unchecked(
vec![
record_wo_desc!("A2", b"XXX--X"),
record_wo_desc!("B3", b"X-X--X"),
record_wo_desc!("C5", b"X---X-"),
record_wo_desc!("D6", b"--XXX-"),
record_wo_desc!("I4", b"--XXX-"),
],
Alphabet::dna(),
);
let msa = MSA::from_aligned(sequences, tree.clone()).unwrap();
for seq in msa.msa.seqs() {
println!("{}: {}", seq.id(), String::from_utf8_lossy(seq.seq()));
}
for map in msa.msa.leaf_maps().values() {
println!("leaf map: {map:?}");
}prints
A2: XXXX
B3: XXX
C5: XX
D6: XXX
I4: XXX
leaf map: [Some(0), None, None, None, Some(1), None]
leaf map: [Some(0), Some(1), Some(2), None, None, Some(3)]
leaf map: [Some(0), None, Some(1), None, None, Some(2)]
leaf map: [None, None, Some(0), Some(1), Some(2), None]
| "ancestral sequences must have the same length as the provided alignment" | ||
| ); | ||
| } | ||
| validate_ancestor_ids(&phylo.tree, &ancestral_seqs)?; |
There was a problem hiding this comment.
copilot added
phylo.tree.node_ids_are_unique()?;
This seems like a valid suggestion to me
| "ancestral sequences must have the same length as the provided alignment" | ||
| ); | ||
| } | ||
| validate_ancestor_ids(&phylo.tree, &ancestral_seqs)?; |
There was a problem hiding this comment.
Like with the validate_taxa_ids it think this sequences object could contain seqs with ids that are in the tree tips set. they would not be detected by validate_ancestor_ids, in from_msa_with_ancestral they are silently ignored. So not quite like the MSA::from_aligned which added the additional seqs to the .seqs anyway rather than silently ignoring it.
| /// Tree is consumed and stored together with the ancestral alignment, because the alignment is only | ||
| /// meaningful in the context of the tree that it was built from. |
There was a problem hiding this comment.
same remark as the comment I added to: from_msa_with_ancestral
| let ancestral_seqs = | ||
| Sequences::with_alphabet_unchecked(ancestral_records, phylo.msa.alphabet()) | ||
| .into_gapless(); |
There was a problem hiding this comment.
I guess in this method:
fn from_msa_with_ancestral_unchecked(
phylo: PhyloInfo<impl Alignment>,
ancestral_seqs: Sequences,
) -> PhyloInfo<Self> {when we check that we have a correct number of sequences and that they match the internal node ids (this is currently not the case as seqs with leaf ids can be in the passed ancestral_seqs), then we dont need to create this vec of ancestral_records in the for loop but can straight up take the passed ancestral_seqs and save it to MASA. we just need to assert the seqs alphabet is the same as the msa alphabet.
| // sim-0_msa_new.fasta as the input msa | ||
| // sim-0_new.newick as the input tree (changed node ids from "x" -> "i_x") | ||
| // sim-0_inferred.fasta is the result for comparison (concatenated alignment from input and ARPIP reconstruction) | ||
| // Note: this dataset has no insertions that are more likely to be higher than the MRCA. |
There was a problem hiding this comment.
Perhaps say that you have tests for the case that insertion is higher than MRCA, so users reading that here know that you also test for that case that is not covered here.
| // Deletion probabilities for each node, indexed by node (single value per branch). | ||
| deletion_probs: Vec<f64>, |
There was a problem hiding this comment.
I dont know how its for you in vscode but for me its nice to have these as /// comments since then I can get the hover text on that variable (also at every place where its used) and read the comment without having to jump to this line.
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub(crate) struct PIPAsrInfo<Q: QMatrix> { |
There was a problem hiding this comment.
Perhaps link the paper where the diagram that you made is in
| // the potential values for f and p are precomputed but can be reset by deletion and insertion processing | ||
| let f_child1 = tmp.f[children[0]].clone(); | ||
| tmp.f[idx] = f_child1.clone().component_mul(&tmp.f[children[1]]); | ||
| tmp.f[idx] *= 1.0 - tmp.deletion_probs[idx]; | ||
| tmp.p[idx] = | ||
| f_child1.component_mul(&tmp.f[children[1]]) * tmp.insert_and_survive_probs[idx]; |
There was a problem hiding this comment.
By the comment you mean that those values:
insert_and_survive_probs: insert_and_survive_probabilities(model, info),
deletion_probs: deletion_probabilities(model, info),are precomputed?
Or you are doing the precomputing here and some time later these (f and p) values might get set to a different value?
There was a problem hiding this comment.
I think yes, perhaps change comment to:
/// precompute potential values for f and p. They might be overwritten by deletion and insertion processing
so that it is clear that the precomputing happens here and was not "precomputing performed somewhere else and the place here is where they are overwritten".
| let ancestral_seqs = | ||
| Sequences::with_alphabet_unchecked(ancestral_records, phylo.msa.alphabet()); | ||
|
|
||
| AA::from_msa_with_ancestral(phylo, ancestral_seqs).unwrap() |
There was a problem hiding this comment.
perhaps also call the unchecked version here?
| // Data from the ARPIP paper, javapip simulation with mu = 0.1 | ||
| // sim-0_msa_new.fasta as the input msa | ||
| // sim-0_new.newick as the input tree (changed node ids from "x" -> "i_x") | ||
| // sim-0_inferred.fasta is the result for comparison (concatenated alignment from input and ARPIP reconstruction) | ||
| // Note: this dataset has no insertions that are more likely to be higher than the MRCA. |
There was a problem hiding this comment.
perhaps also add this to a readme in the data/pip/arpip/readme
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
| #[cfg(test)] | |
| #[cfg(test)] | |
| #[cfg_attr(coverage, coverage(off))] |
i guess this is also missing in:
- src/parsimony/matrices/mod.rs:502
- src/parsimony_presence_absence/mod.rs:127
- src/alignment/sequences.rs:412
- src/tkf_model/tkf91.rs:221
- src/tkf_model/mod.rs:196 (tkf_numerical_tests)
| phylo: PhyloInfo<impl Alignment>, | ||
| ancestral_seqs: Sequences, | ||
| ) -> Result<PhyloInfo<Self>> { | ||
| if !ancestral_seqs.alphabet().eq(phylo.msa.alphabet()) { |
There was a problem hiding this comment.
| if !ancestral_seqs.alphabet().eq(phylo.msa.alphabet()) { | |
| if ancestral_seqs.alphabet() != phylo.msa.alphabet() { |
| -(-pip.mu() * node.blen).exp_m1() / pip.mu() | ||
| }) * common_factor | ||
| }) | ||
| .collect::<Vec<f64>>() |
There was a problem hiding this comment.
| .collect::<Vec<f64>>() | |
| .collect() |
| #[test] | ||
| fn insertion_at_i5() { | ||
| let tree = | ||
| tree!("(((L3:1.0,L4:1.0)I2:1.0,(L6:0.001,L7:1.0)I5:10000.0)I1:1.0,L8:1.0)I0:0.0;"); |
There was a problem hiding this comment.
In this test and similar ones we dont need the branches to be that short and that long, more moderate lens are sufficient, but could also keep it like this.
Simplified sequence/node match checks with helper function `check_node_sequence_ids_match`. Include validation that internal node ids are actually set (not empty) for `validate_ids_with_ancestors` and `validate_ancestor_ids`
Added tests for `ensure_ancestral_ids_set` and `check_node_sequence_ids_match` helpers


Implementation of indel event reconstruction under PIP and refactoring.
ARPIP implementation:
asrmodule topip_modelwith aPIPIndelsstruct that implementsAncestralSequenceReconstruction.API Improvement:
from_msa_with_ancestralandfrom_msa_with_ancestral_uncheckedmethods to theAncestralAlignmenttrait, to construct aPhyloInfowith ancestral sequences from an existing alignment and a set of ancestral sequences.from_msa_with_ancestralincludes default implementation with validation checks for matching alphabets and sequence lengths between MSA and ancestral sequences and matching node IDs.Utility Functions and Refactoring:
mapping_to_bitsutility function, which converts a sequence mapping to aFixedBitSet(will be potentially refactored away when theMappingtype changes toFixedBitSetfromVec<Option<usize>>).alphabetmethod in theAlignmenttrait and its implementations to always return a reference to a staticAlphabet.validate_ancestor_ids) and adjusted imports accordingly.warn_on_extinct_columnsfunction to provide users with warnings when columns in the alignment go extinct in all leaf sequences, suggesting the use ofremove_extinct_columns.