One sided RF distance #76
Conversation
historydag/utils.py
Outdated
| @@ -682,6 +682,58 @@ def edge_func(n1, n2): | |||
| return kwargs | |||
|
|
|||
|
|
|||
| def one_sided_rfdistance_funcs(reference_dag: "HistoryDag"): | |||
There was a problem hiding this comment.
I think this looks great! I think the argument reference_dag should be called reference_history, since even though it's a history DAG object, it's expected to be tree-shaped (hence an history).
Also, we should make it very clear in the docstring whether the reference_history is expected to be multifurcating or not, when we're trying to detect resolutions of multifurcating trees.
historydag/dag.py
Outdated
| ref_history = dag_copy.sample() | ||
| return 2 * self.optimal_rf_distance(ref_history, optimal_func=max) | ||
|
|
||
| def optimal_one_sided_sum_rf_distance( |
There was a problem hiding this comment.
I think this should be called just optimal_one_sided_rf_distance since we're not taking a sum over anything... unless I'm misunderstanding!
There was a problem hiding this comment.
I think the method returns something like the sum of one-sided RF distances rather than a single distance. I tried running it one some examples, and it returns a number that is bigger than the one returned by optimal_rf_distance.
Also, from skimming the code in utils.one_side_rfdistance_funcs it looks pretty close to utils.sum_rfdistance_fucs
|
This great work was incorporated into #80 so I'm closing this PR |
No description provided.