adds LigandNetwork.reduce_graph
#320
Conversation
|
Hello @richardjgowers! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-05-15 06:22:37 UTC |
richardjgowers
force-pushed
the
LigandNetwork_delete_edges
branch
from
f7e5072 to
1a33b8c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
=======================================
Coverage 98.50% 98.51%
=======================================
Files 37 37
Lines 2147 2156 +9
=======================================
+ Hits 2115 2124 +9
Misses 32 32 ☔ View full report in Codecov by Sentry. |
gufe/ligandnetwork.py
Outdated
| def remove_edges(self, edges: Union[LigandAtomMapping, list[LigandAtomMapping]]) -> LigandNetwork: | ||
| """Create a new copy of this network with some edges removed | ||
|
|
||
| Note that this will not remove any nodes, potentially resulting in |
There was a problem hiding this comment.
How big of deal is this? Is it worth adding the complexity of checking to make sure we don't allow that? I can't remember what the rules are, if we allow creating a network that is disconnected, then I don't think we need a check, but if we require a connected network, then we want to make sure we don't let users put things into a inconsistent state.
There was a problem hiding this comment.
This is a good suggestion, I think :)
There was a problem hiding this comment.
Yeah I was 50/50 on what is best here. Currently disconnected graphs are "allowed" in a data structure sense, but they're often undesirable. It wouldn't be impossible to find and remove orphan nodes, but then the method isn't strictly "remove edges". I think I'll chicken out and add a kwarg that allows either, default being remove orphans.
There was a problem hiding this comment.
It wouldn't be impossible to find and remove orphan nodes
Ah yes, I was suggesting a more gentle "throw an error if removing this edge creates an orphan" or something so it can stay focused and do what it says on the tin, so I would be happy with kwargs that control things like, raising an error or not if it would create an orphan, and/or remove orphan(s) (this will be tricking if removing a edge creates 2 disconnect graphs, which one gets remove? the one with more nodes? -- maybe we can support the case where if it creates a single orphan, the default is to raise an error, but that can be suppressed, and additionally with another kwarg the orphan can be removed)
Co-authored-by: Benjamin Ries <[email protected]>
To create the inverse method of `enlarge_graph`, created `reduce_graph`. This new method gives a superset of the functionality proposed in `remove_edges`. Also made a bunch of consistency edits to docstrings while I was at it.
dotsdl
changed the title
LigandNetwork.reduce_graph
Not strictly necessary, but for consistency
|
@atravitz this is now ready for review! I added |
There was a problem hiding this comment.
My hesitation about naming here is that "reduce" in graph theory sort of implies that nodes are preserved (e.g. https://networkx.org/documentation/stable/reference/algorithms/generated/networkx.algorithms.dag.transitive_reduction.html). Could this be named remove or remove_from (to mirror networkx's languag) to be more clear?
Also, is the reason for handling nodes and edges in the same function to avoid making copies of the graph unnecessarily?
Committing suggestions from @atravitz. Co-authored-by: Alyssa Travitz <[email protected]>
|
Thanks @atravitz! I'm trying to maintain some symmetry to |
"shrink" works, but I'm fine with keeping |
|
@atravitz after talking with @ianmkenney, we like |
that works for me! |
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
move this from Konnektor into gufe, it's very useful