tree almost_eq by MattesMrzik · Pull Request #133 · acg-team/rust-phylo

MattesMrzik · 2026-01-19T13:03:00Z

Implemented a almost_eq method to compare trees ignoring internal representations.
Closes #78. As we not want to directly implement Eq since we are still comparing f64 fields for which Eq is not implemented.

MattesMrzik · 2026-01-19T13:03:38Z

Perhaps we want to return a Result instead of a bool? Then people can easily see how the two trees differ, by looking at the returned error msg.

MattesMrzik · 2026-01-19T13:30:38Z

Had to change a Newick string in a test. See #134

codecov · 2026-01-19T14:54:54Z

Codecov Report

❌ Patch coverage is 84.90566% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.67%. Comparing base (527040d) to head (ab2a8b8).
⚠️ Report is 23 commits behind head on develop.

Files with missing lines	Patch %	Lines
phylo/src/tree/mod.rs	84.90%	8 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #133      +/-   ##
===========================================
+ Coverage    96.32%   96.67%   +0.35%     
===========================================
  Files           32       43      +11     
  Lines         4355     5029     +674     
===========================================
+ Hits          4195     4862     +667     
- Misses         160      167       +7

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

Pull request overview

This PR implements an almost_eq method for phylogenetic trees to compare them while allowing small differences in branch lengths (controlled by an epsilon parameter). The method ignores internal representations and node indices, focusing on tree topology and branch lengths. This addresses issue #78 by providing an approximate equality comparison without implementing the Eq trait (which would be problematic due to f64 fields).

Changes:

Added almost_eq method to Tree struct that compares trees by traversing from each leaf to the root
Added comprehensive test coverage with 10 new test cases covering various scenarios
Minor fixes including whitespace correction in a test string, test function rename, and added documentation comment

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
phylo/src/tree/mod.rs	Implements the `almost_eq` method with topology and branch length comparison, adds `log::warn` import, adds documentation comment for `is_subtree`, and includes a TODO comment
phylo/src/tree/tests.rs	Adds 10 comprehensive test cases for `almost_eq`, fixes whitespace in a test Newick string, and demonstrates usage of the new method
phylo/src/asr/tests.rs	Renames test function from `reconstruct_ancestral_seqs_n_seqs_not_same_as_leaves` to `asr_n_seqs_not_same_as_leaves` for brevity

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-20T09:38:00Z

    postorder: Vec<NodeIdx>,
    preorder: Vec<NodeIdx>,
    leaf_ids: Vec<String>,
+    // TODO: what is meant by this?


The comment "TODO: what is meant by this?" should be removed or resolved before merging. This TODO appears to be unrelated to the PR's purpose and adds confusion to the codebase. If the meaning of the complete field is unclear, it should be documented properly rather than leaving a TODO comment.

Suggested change

// TODO: what is meant by this?

/// Indicates whether the tree has been fully constructed and its traversal

/// orders and metadata are up to date.

Co-authored-by: Copilot <[email protected]>

junniest

This is a much more complex problem (comparing trees) than it looked on the surface :D I think we still need to think how to tackle it consistently and properly.

junniest · 2026-02-02T13:53:14Z

    postorder: Vec<NodeIdx>,
    preorder: Vec<NodeIdx>,
    leaf_ids: Vec<String>,
+    // TODO: what is meant by this?


hahaha I agree with copilot on this, and its suggestion is correct. This was a field meant for the situation when the tree is being constructed and not nodes are all filled in, traversals are not initialised, etc. It's not a great way to handle that and I will try to refactor this out so that there's no way to get an incomplete tree in the first place. See #121.

junniest · 2026-02-02T13:55:46Z

        partitions
    }

+    /// Returns true if `query` is in the subtree rooted at `node`.


This could actually be a pub method, I think. In that case, it would be nice to also have a docstring example. If you don't have the time to change it I will do that in #121.

junniest · 2026-02-02T14:10:25Z

+    ///
+    /// # Panics
+    ///
+    /// Panics if the ids of the leaves are not unique;


I don't think that equality should check whether IDs are unique in each tree, since this is a different kind of check. It's a check on tree validity rather than on equality. I would say that you should assume that if the trees have already been created by this point (and they have), the trees themselves will be valid.

Actually, funnily enough, this made me add a new test for the newick parser and it does not fail when there are duplicate ids. I will fix that asap (#146)

junniest · 2026-02-02T14:13:05Z

 fn newick_parse_whitespace() {
    let trees = from_newick(
-        "     (     (((  (A:1   , B  :   1.0)  \n \n F:1,C:2.0   )G:1,D:3)H:+1.0  ,  E:4)   I:1)\n;\n   ",
+        "     (     (((  (A:1   , B:   1.0)  \n \n F:1,C:2.0   )G:1,D:3)H:+1.0  ,  E:4)   I:1)\n;\n   ",


I am a bit confused here. Why did you end up removing the whitespace? At the moment (at least for me) this test already works just fine (the parser does not include whitespace in the name).

Thats weird. With this PR the ids are compared which are "B " for the first tree and "B" for the second (before this PR ids where not checked in the test). See #136 that fixed that. So when #136 is merged this whitespace can be added back.

junniest · 2026-02-02T14:18:21Z

+            return false;
+        }
+        if self.n != other.n
+            || self.complete != other.complete


I will remove this flag, so you can remove this check. A created tree should always be complete and valid.

The other thing is that self.n must be equal to self.nodes.len(), so this will be a duplicate check which can also be removed.

No its not. self.n is the number of leaves not nodes. If we assume that the tree struct is valid, is rooted and binary, then if the ns are equal it follows that the nodes.length also match.

junniest · 2026-02-02T14:55:08Z

+                    return false;
+                }
+                // checking the branch lengths
+                if n1.blen.is_nan() || n2.blen.is_nan() || (n1.blen - n2.blen).abs() > epsilon {


Here again, any NaNs with always fail the last comparison, so you only need that one for this check to work.

junniest · 2026-02-02T14:59:33Z

+            }
+        }
+
+        // not comparing Tree.dirty as it is crate internal


Uuuugh, I think the dirty bitset should not be a part of the tree, now that I think about it... But I am not sure what would be a better place for it.

junniest · 2026-02-02T15:04:36Z

+                    None => None,
+                };
+            }
+            if n2_option.is_some() {


And the trees need to be rooted the exact same way, which can be both good and bad. I am not sure if it's worth trying to compare trees with different rootings, but if we expect the rooting to be the same I think it should be said in the doctring.

junniest · 2026-02-02T15:05:25Z

+    /// Compares two trees for approximate equality, allowing for small differences in branch
+    /// lengths. This comparison does not care about the order of nodes in the internal representation
+    /// or correspondence of [`Node`]s and [`NodeIdx`]s. The order of children of a node is ignored,
+    /// i.e., they are compared as sets. It returns `false` in the case of `NaN` branch lengths.


The last sentence isn't necessary, imo (see my comments about NaNs below). NaNs should not appear in any case, but if they do the whole structure is invalid anyway.

junniest · 2026-02-02T15:05:41Z

+}
+
+#[test]
+fn almost_eq_trees_with_wrong_internal_id() {


This is curious. Technically, if we have the same structure (same leaf clustering) the internal node names shouldn't matter. I am not sure how to make that into a rule here and whether we should bother with it at all, but it might pop up eventually.

MattesMrzik added 4 commits January 16, 2026 11:18

added ascii tree to illustrate spr move

4ba34cd

undo the spr ascii on the develop branch

746e018

first working version of eq with tests

3da6b48

cleanup

4f51d13

removed whitespace from id in test newick parse

f7e3f20

MattesMrzik requested review from Copilot and junniest January 20, 2026 09:33

Copilot started reviewing on behalf of MattesMrzik January 20, 2026 09:33 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

MattesMrzik and others added 5 commits January 20, 2026 11:24

Update phylo/src/tree/mod.rs

6e4d7b8

Co-authored-by: Copilot <[email protected]>

Update phylo/src/tree/mod.rs

21136f6

Co-authored-by: Copilot <[email protected]>

Update phylo/src/tree/mod.rs

3917629

Co-authored-by: Copilot <[email protected]>

Update phylo/src/tree/tests.rs

96f3570

Co-authored-by: Copilot <[email protected]>

Update phylo/src/tree/tests.rs

ab2a8b8

Co-authored-by: Copilot <[email protected]>

junniest reviewed Feb 2, 2026

View reviewed changes

	// TODO: what is meant by this?
	/// Indicates whether the tree has been fully constructed and its traversal
	/// orders and metadata are up to date.

Uh oh!

Conversation

MattesMrzik commented Jan 19, 2026

Uh oh!

MattesMrzik commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattesMrzik commented Jan 19, 2026

Uh oh!

codecov Bot commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junniest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattesMrzik Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MattesMrzik commented Jan 19, 2026 •

edited

Loading

codecov Bot commented Jan 19, 2026 •

edited

Loading

MattesMrzik Feb 3, 2026 •

edited

Loading