Ambiguity fix #20
Ambiguity fix #20
Conversation
Added try_idx method to get the idx of the node in the tree by its id, checked method that returns a result. Changed idx() to be unchecked and panic if the node isn't there.
Changed create_postorder to compute_postorder and set it to recompute each time when called as otherwise it does not get triggered when the tree changes.
Added methods for spr and unchecked spr moves as well as getting node sibling
Changed preorder_subroot to always need a subroot index instead of Option, as it was just adding unnecessary complexity.
Fixed SPR bug where the parent of the pruned node didn't get the right new parent set. Added test that helped find the bug.
Added tests to verify that the likelihood computation for gaps and ambiguous characters matches PhyML for substitution models.
Added tests to check optimised model parameters in K80 and HKY against the values optimised by PhyML
Added two tests for comparing the topology optimisation and likelihoods on gapped example with fixed or empirical frequencies
Refactored topology tests to either optimise or use precomputed results if available
| @@ -24,14 +24,14 @@ fn check_likelihood_opt_k80() { | |||
| .build() | |||
| .unwrap(); | |||
| let model = DNASubstModel::new(K80, &[4.0, 1.0]).unwrap(); | |||
| let unopt_logl = model.cost(&info); | |||
| let unopt_logl = model.cost(&info, false); | |||
There was a problem hiding this comment.
not clear from the code, what is that false. consider adding a method that would make it clear e.g. cost_with_blah
| } | ||
|
|
||
| impl<'a, EM: PhyloCostFunction + Clone> PhyloOptimiser<'a, EM> for TopologyOptimiser<'a, EM> { | ||
| fn new(model: &'a EM, info: &PhyloInfo) -> Self { |
There was a problem hiding this comment.
I recommend passing values instead of references here, and doing .clone() outside. This will make things cleaner
| let mut moves: Vec<(f64, Tree)> = Vec::with_capacity(regraft_locations.len()); | ||
| for regraft_branch in ®raft_locations { | ||
| let mut new_info = info.clone(); | ||
| new_info.tree = info.tree.rooted_spr(prune_branch, regraft_branch).unwrap(); |
There was a problem hiding this comment.
nit: something feels wrong here. is it certain this place cannot fail? I'd recommend error handling. probably ? should be better than just unwrap
There was a problem hiding this comment.
also it's a strange clone() here. I would recommend moving this type of "cloning" to the tree inteself. e.g. clone_for_tree
There was a problem hiding this comment.
This should never fail, but yes, ? is definitely better than unwrap().
There was a problem hiding this comment.
agreed for the second point, I am cloning a big structure that I don't need to clone at all. I will refactor that.
| curr_cost = o.final_logl; | ||
| info = o.i; | ||
| } | ||
| info.tree.clean(true); |
There was a problem hiding this comment.
it isn't clear what this true is here
Fixing the bug where ambiguous characters were not being handled correctly, now the handling matches phyml 1 to 1.