Closed clarisw closed 1 year ago
^^ I figured out where the math was wrong, and pushed the changes needed to make the test run.
Here's a list of to-do's, just so we can keep track (I know we've talked about some of these already):
IntState
to shift by a constant automatically. Notice that I had to change the constant computation slightly in sum_rf_distance
.sum_rf_distance
to optimal_sum_rf_distance
for consistency with optimal_rf_distance
, and add the optimal_func
keyword argument (just like in optimal_rf_distance
to allow the user to specify whether optimal means max or min, or something else.trim_optimal_sum_rf_distance
method, analogous to trim_optimal_rf_distance
, and make sure the docstring mentions that trimming to minimum sum rf distance is finding 'median' topologies, and trimming to maximum sum rf distance is finding topological outliers.This looks great, Claris! Feel free to merge it whenever you're ready.
^ I just rebased, to make things easier!
This is looking great, Claris! The test still isn't passing, but I rewrote it a bit to make it more clear what's supposed to happen, and I fixed a couple of minor things.
I'm not quite sure what's going on here, but it's possible that either some math is incorrect (for example in the issue #39 ), or there's something wrong with
count_nodes
, etc.A couple of comments:
rooted
keyword argument for the sum RF distance stuff, because we're going to just compute rooted RF distance. But the existing RF distance code is unrooted by default, so we have to specify that when computing expected values in the tests.HistoryDag.sum_rf_distance
toHistoryDag.optimal_sum_rf_distance
because that's what it's doing: finding the optimal sum RF distance for any tree inself
to trees inreference_dag
. We should therefore also include the argumentoptimal_func
to specify whether we want min or max sum RF distance, similar to inHistoryDag.optimal_rf_distance
.