Adding B1 balance index and tests

[jeremyguez]

Here is the PR for B1 balance index. Compared to #2251 I just added a round function at the end. @jeromekelleher

[codecov]

Codecov Report

Merging #2281 (72474ec) into main (3e70389) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2281   +/-   ##
=======================================
  Coverage   93.29%   93.30%           
=======================================
  Files          27       27           
  Lines       26089    26097    +8     
  Branches     1172     1175    +3     
=======================================
+ Hits        24341    24349    +8     
  Misses       1718     1718           
  Partials       30       30           

Flag	Coverage Δ
c-tests	92.23% <ø> (ø)
lwt-tests	89.05% <ø> (ø)
python-c-tests	71.86% <12.50%> (-0.04%)	⬇️
python-tests	98.88% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/tskit/trees.py	98.05% <100.00%> (+<0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e70389...72474ec. Read the comment docs.

[jeromekelleher]

Looks great! I don't understand why we're rounding though?

[jeremyguez]

Looks great! I don't understand why we're rounding though?

It's because of the tests, having something like this didn't look clean (and I felt it could lead to errors):

def test_b1(self):
        assert self.tree().b1_index() == 1.8333333333333333

And I wasn't sure this was clean either, since it's not testing the output of the method itself:

def test_b1(self):
        assert round(self.tree().b1_index(),3) == 1.833

But I guess there is a clean general way of working around tests in that case?

[jeremyguez]

By the way: I wrote in the doc that the output is a float, but actually in some cases, like for the empty tree, the output is 0 which is an int. Should we force the result to be a float, so it's always the same output type?

[jeromekelleher]

Ah, I see. I think the usual way to handle this would be something like

def test_b1(self):
        assert self.tree().b1_index() == pytest.approx(1.8333)

By the way: I wrote in the doc that the output is a float, but actually in some cases, like for the empty tree, the output is 0 which is an int. Should we force the result to be a float, so it's always the same output type?

Good point. The cleanest way to fix this would be to set total = 0.0 at the top of the loop, which will force it to be a float.

[jeremyguez]

By removing the rounding I get now this kind of errors:

assert tree.b1_index() == b1_index_definition(tree)
E           assert 19.4718253968254 == 19.471825396825395

I guess I should use python.approx() in all tests?

[jeromekelleher]

Use pytest.approx, like

assert tree.b1_index() == pytest.approx(b1_index_definition(tree))

[jeremyguez]

I put pytest.approx(1.833, rel=1e-3), as the default is using rel=1e-6.

Is it an issue if the definition method output is not always a float, or should I force it to float too? The tests do pass without it.

[jeromekelleher]

Do the tests not pass at the default tolerance? I would have expected the values to be very close.

Is it an issue if the definition method output is not always a float, or should I force it to float too? The tests do pass without it.

that's fine, it doesn't really matter.

[jeremyguez]

Do the tests not pass at the default tolerance? I would have expected the values to be very close.

For this test the default relative tolerance is indeed working, as the difference is very small:

assert tree.b1_index() == pytest.approx(b1_index_definition(tree))

But for this one, the default 1e-6 relative tolerance is too small compared to the difference between 1.833333.. and 1.833:

assert self.tree().b1_index() == pytest.approx(1.833)

So I had two solutions that pass the test:

assert self.tree().b1_index() == pytest.approx(1.833, rel=1e-3)

or

assert self.tree().b1_index() == pytest.approx(1.8333333)

The first one seemed cleaner to me.

[jeromekelleher]

Ah, I see. When you're comparing with a fixed value either way is good. Whatever you prefer.

[jeromekelleher]

LGTM, spotted one issue in docstring

[jeromekelleher]

Delete (rounded)