Enum cycle in an undirected graph (and fix bug in yen_k_shortest_simple_path algorithm) #40217
Conversation
|
Documentation preview for this PR (built with commit 0418243; changes) is ready! 🎉 |
src/sage/graphs/connectivity.pyx
Outdated
| @@ -812,6 +813,44 @@ def blocks_and_cuts_tree(G): | |||
| g.add_edge(('B', bloc), ('C', c)) | |||
| return g | |||
|
|
|||
| def biconnected_components_subgraphs(G): | |||
| """ | |||
There was a problem hiding this comment.
It's best to use r""" here.
src/sage/graphs/cycle_enumeration.py
Outdated
| raise ValueError("negative weight is not allowed") | ||
|
|
||
| if algorithm == 'A': | ||
| if not self.is_directed(): |
There was a problem hiding this comment.
I think that checks on input parameters should be done before any call to copy or decomposition into biconnected components.
src/sage/graphs/cycle_enumeration.py
Outdated
| if starting_vertices is None: | ||
| starting_vertices = self | ||
|
|
||
| if self.is_directed(): |
There was a problem hiding this comment.
I think we can avoid this part of the code
if self.is_directed():
# Since a cycle is always included in a given strongly connected
# component, we may remove edges from the graph
sccs = self.strongly_connected_components()
d = {}
for id, component in enumerate(sccs):
for v in component:
d[v] = id
h = copy(self)
h.delete_edges((u, v) for u, v in h.edge_iterator(labels=False) if d[u] != d[v])
else:
h = copy(self)Indeed, for algorithm 'B', the first step is to decompose the graph h into strongly connected / biconnected components. So there is no added value to make a copy of self and/or remove it some edges.
It might be different for algorithm 'A', but we must be careful. Notice that a cut vertex may belong to several strongly connected components, so we cannot decompose the graph into strongly connected components first. We can remove arcs between components. So may be this part could be moved into the block for algorithm 'A'.
| h = None | ||
| for component in components: | ||
| if component.has_edge(edge): | ||
| h = component |
There was a problem hiding this comment.
add a break to exist the loop early. Actually, you can use (without h = None):
for component in components:
if component.has_edge(edge):
h = component
break
else:
# edge connects two strongly connected components, so
# no simple cycle starting with edge exists.
return
| yield [vertex] | ||
| # First we remove vertices and edges that are not part of any cycle | ||
| if remove_acyclic_edges: | ||
| sccs = self.strongly_connected_components() |
There was a problem hiding this comment.
further minor improvement: if the graph is strongly connected (so len(sccs) == 1), we can do h = self.
src/sage/graphs/cycle_enumeration.py
Outdated
| # Since a cycle is always included in a given strongly connected | ||
| # component, we may remove edges from the graph | ||
| sccs = self.strongly_connected_components() | ||
| d = {} |
There was a problem hiding this comment.
here also, if the graph is strongly connected, there is no need for a copy.
kappybar
force-pushed
the
enum_cycle_undirected
branch
from
4b4a3e3 to
9057ffd
Compare
There was a problem hiding this comment.
I don't think it is necessary to expose methods _all_cycles_iterator_vertex and _all_simple_cycles_iterator_edge in DiGraph. These are internal method users can always import these methods if needed. Of course, if these methods are not exposed, you will have to import them in your methods and in relevant doctests.
|
in the description of this PR, you must add the dependency on #40248. |
|
If I stop exposing method h._all_cycles_iterator_vertex(v,
starting_vertices=starting_vertices,
simple=simple,
rooted=rooted,
max_length=max_length,
trivial=trivial,
remove_acyclic_edges=False,
weight_function=weight_function,
by_weight=by_weight,
check_weight=check_weight,
report_weight=True)to _all_cycles_iterator_vertex(h, v,
starting_vertices=starting_vertices,
simple=simple,
rooted=rooted,
max_length=max_length,
trivial=trivial,
remove_acyclic_edges=False,
weight_function=weight_function,
by_weight=by_weight,
check_weight=check_weight,
report_weight=True)for each function call. |
|
Yes, this is what I mean, but I don't know if it is a good idea or not... Well, let it as is. It's working. |
|
I cannot determine which is good neither ... It might be fine to leave the current implementation as it is. |
sagemathgh-40217: Enum cycle in an undirected graph (and fix bug in yen_k_shortest_simple_path algorithm) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> Add implementation of enumeration of cycles in an undirected graph. Specifically, I make ```sage/graphs/cycle_enumeration.py``` and modify + ```~sage.graphs.cycle_enumeration._all_simple_cycles_iterator_edge``` + ```~sage.graphs.cycle_enumeration.all_cycles_iterator``` + ```~sage.graphs.cycle_enumeration.all_simple_cycles``` In implementing these funcionts, I fix bug in ```~sage.graphs.path_enumeration.yen_k_shortest_simple_paths```. This bug occurs when there is no path between source and target. Also, I add ```~sage.graphs.connectivity.biconnected_components_subgraphs```. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies sagemath#40248 <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#40217 Reported by: Yuta Inoue Reviewer(s): David Coudert
Add implementation of enumeration of cycles in an undirected graph.
Specifically, I make
sage/graphs/cycle_enumeration.pyand modify~sage.graphs.cycle_enumeration._all_simple_cycles_iterator_edge~sage.graphs.cycle_enumeration.all_cycles_iterator~sage.graphs.cycle_enumeration.all_simple_cyclesIn implementing these funcionts, I fix bug in
~sage.graphs.path_enumeration.yen_k_shortest_simple_paths.This bug occurs when there is no path between source and target.
Also, I add
~sage.graphs.connectivity.biconnected_components_subgraphs.📝 Checklist
⌛ Dependencies
#40248