Small parsimony #276
Small parsimony #276
Conversation
…e in and output TreeData trees
…to allow pass-in of TreeData tree
…peiaTree but now takes in any TreeLike
…CassiopeiaTree but now can take in TreeData
…ut now can take in any treelike
…e but now can take in any treelike
…reeData, using new helper functions in utils.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.0.0 #276 +/- ##
==========================================
- Coverage 84.94% 84.71% -0.24%
==========================================
Files 76 76
Lines 5773 5881 +108
==========================================
+ Hits 4904 4982 +78
- Misses 869 899 +30
🚀 New features to boost your workflow:
|
…ttribute_treelike to work with nx.digraphs. rewrite fitch_hartigan_bottom_up to be in terms of nx.digraphs
colganwi
left a comment
colganwi
left a comment
There was a problem hiding this comment.
@gloriayma were getting close! I've updated the tests to use pytest and treedata. Please review the failing tests and update your code accordingly. You should be updating the tests as you go to ensure that all added functionality is covered.
The major changes here are converting fitch_hartigan_bottom_up and fitch_hartigan_top_down to private methods that take an nx.DiGraph as input. The fitch_hartigan, score_small_parsimony, and fitch_count functions should all have
g, _ = _get_digraph(tree, treedata_key)
as the first line. This change will make these functions easier to implement since you won't have to think about the type of the tree variable.
| Returns: | ||
| A new CassiopeiaTree if the copy is set to True, else None. | ||
| A new CassiopeiaTree/TreeData/nx DiGraph + meta_df if the copy is set to True, else None. |
There was a problem hiding this comment.
Does not return meta_df so this should be removed from the docstring
| return tree if copy else None | ||
|
|
||
|
|
||
| def fitch_hartigan_bottom_up( |
There was a problem hiding this comment.
Let's make this a private _ method. It should only accept nx.Digraph as input. So you will need to call _get_digraph in fitch count rather than fitch_hartigan_bottom_up.
This will simplify the logic and remove redundant code.
| if is_numeric_dtype(meta): | ||
| raise CassiopeiaError("Meta item is not a categorical variable.") | ||
|
|
||
| if not is_categorical_dtype(meta): |
There was a problem hiding this comment.
DeprecationWarning: is_categorical_dtype is deprecated and will be removed in a future version. Use isinstance(dtype, pd.CategoricalDtype) instead
| return tree if copy else None | ||
|
|
||
|
|
||
| def fitch_hartigan_top_down( |
There was a problem hiding this comment.
Same as bottom_up. Private method that accepts nx.Digraph
|
|
||
| parsimony = 0 | ||
| for parent, child in cassiopeia_tree.depth_first_traverse_edges(source=root): | ||
| for parent, child in tree.depth_first_traverse_edges(source=root): |
There was a problem hiding this comment.
If tree is a TreeData object this will fail because TreeData does not have a depth_first_traverse_edges method.
| if root != cassiopeia_tree.root: | ||
| cassiopeia_tree.subset_clade(root) | ||
| if root != tree.root: | ||
| tree.subset_clade(root) |
There was a problem hiding this comment.
Nx does not have a subset_clade method. Use this instead:
tree = tree.subgraph(nx.descendants(tree, root) | {root}).copy()
| ) | ||
|
|
||
|
|
||
| def _set_attribute_treelike( |
There was a problem hiding this comment.
I don't think we need this helper since we will always be working with nx.DiGraphs
tree.nodes[attribute_name] = value
| raise TypeError("Unsupported tree type. Must be CassiopeiaTree or TreeData.") | ||
|
|
||
|
|
||
| def _get_attribute_treelike(tree: TreeLike, node: str, attribute_name: str) -> Any: |
There was a problem hiding this comment.
Again this is easy with nx
| raise TypeError("Unsupported tree type. Must be CassiopeiaTree or TreeData.") | ||
|
|
||
|
|
||
| def _get_children_treelike(tree: TreeLike, node: str, tree_key: str | None = None) -> list[str]: |
There was a problem hiding this comment.
This is a one liner with nx
list(G.successors(node))
|
tests currently failing because fitch_hartigan doesn't support TreeData correctly. |
Rewrite of small_parsimony in tools module to work with TreeData