adds LigandNetwork.remove_edges #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.94% 98.94%
=======================================
Files 36 36
Lines 1988 1997 +9
=======================================
+ Hits 1967 1976 +9
Misses 21 21 ☔ View full report in Codecov by Sentry. |
| @@ -0,0 +1,23 @@ | |||
| **Added:** | |||
|
|
|||
| * added LigandNetwork.remove_edges | |||
There was a problem hiding this comment.
+1 to this getting into the change log
| 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]>
move this from Konnektor into gufe, it's very useful