Adds ability to merge two IPPMs #503
Conversation
- To do this I changes the named tuple to a class.
# Conflicts: # demos/demo_ippm.ipynb # kymata/io/atlas.py # kymata/io/json.py # kymata/ippm/graph.py # kymata/ippm/ippm.py
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
kymata/ippm/graph.py
Outdated
| self_nodes = {node.node_id: node for node in self.graph_full.nodes()} | ||
| other_nodes = {node.node_id: node for node in other.graph_full.nodes()} | ||
|
|
||
| overlapping_node_ids = set(self_nodes.keys()) & set(other_nodes.keys()) |
There was a problem hiding this comment.
This is (GenAI) relying on the assumption that IPPMNode.__hash__() is computed based only on the ID, which is (I think) no longer true as of upstream changes, and is (thus demonstrated) generally a fragile assumption. I will replace this check with general checks about overlapping transforms, which currently won't be supported.
… `IPPMGraph.serial_sequence` use `set`s of transforms, not `list`s
|
Still todo are checklist items in description |
kymata/ippm/graph.py
Outdated
| combined_ctl = CandidateTransformList(combined_hierarchy) | ||
|
|
||
| # Create new graph by directly merging NetworkX graphs | ||
| merged_graph_full = DiGraph() |
There was a problem hiding this comment.
There should be no need to rebuild the graph from scratch when the graph-building logic is already in the IPPMGraph constructor. I've rewritten this to reuse that. As a bonus, it's much shorter :) Needs review and testing.
Implements:
Tasks
CandidateTransformListsIPPMGraphsIPPMs