Initial framework for balance metrics and tests #2246
Conversation
c512e79 to
36735e4
Compare
Codecov Report
@@ Coverage Diff @@
## main #2246 +/- ##
=======================================
Coverage 93.31% 93.31%
=======================================
Files 27 27
Lines 26025 26035 +10
Branches 1166 1170 +4
=======================================
+ Hits 24284 24294 +10
Misses 1711 1711
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
36735e4 to
e7a9bd0
Compare
|
@jeremyguez would you mind taking a quick look here to see what you think? Are there any other tree topology examples we should include here? The idea is that we'll add tests for each of the balance metrics to the classes as we add them, so we can see the what the values are for these simple cases, easily. |
| assert tree.sackin_index() == sackin_index_definition(tree) | ||
|
|
||
|
|
||
| class TestBalancedBinaryOdd: |
There was a problem hiding this comment.
There's quite a bit of repeated boilerplate here for what is looping over several different topologies. If we're doing #1804 then should these trees be defined in examples.py then this file is a test that checks a dict of tree_name->value pairs as a single parameterised test?
There was a problem hiding this comment.
There's a tension here though between avoiding repetition and being able to see what you're testing. I find it really helpful to be able to just look at the example and see what it is your testing, and see what the result should be. It helps a lot to be able to fine-tune the examples for specific things that you're doing, too, rather than trying to come up with a universal set of examples that cover all possibilities.
There was a problem hiding this comment.
Also, might not be obvious here but the idea is to put test functions for all the other metrics into these classes, so the boilder-plate fraction will go down quite a bit.
|
@jeromekelleher |
|
OK, great, thanks @jeremyguez. I don't think there's much need to have mixed arity in a tree, as these examples are really just to make it easy for us to see what the answer is in some representative situations. The systematic testing is done in the pytest.mark.parametrize where we compare the definition with the implementation over lots of examples. OK, going to merge this then. Are you happy to pick this up from here @jeremyguez? (I'll do the C version of this now, so you have examples to follow there) |
e7a9bd0 to
cb057e7
Compare
|
@jeromekelleher Yes, I started to look at #2249. |
This is the first step for adding tree balance metrics as discussed in #2245.
If we agree with this approach, we can follow up with some specific issues to (a) add other metrics; (b) document them; and (c) implement them in C.