Improve PolynomialSequence.connection_graph() implementation #35512
Conversation
Before the change:
sage: g = Graph()
sage: g.add_clique([1,2])
sage: g.add_clique([3])
sage: g.vertices(sort=True)
[1, 2]
After the change:
sage: g = Graph()
sage: g.add_clique([1,2])
sage: g.add_clique([3])
sage: g.vertices(sort=True)
[1, 2, 3]
Simplify the code; also, the previous code has to iterate over variables of the sequence twice (this is really bad before sagemath#35510) Moreover, now we add a clique between the variables of each polynomial, so it agrees with the description (the code before used to add just a spanning tree of the clique -- a star). This makes this method a little bit slower for the purposes of `connected_components()` (for which adding a star is equivalent). However, sagemath#35518 will rewrite `connected_components()` without using `connection_graph()` so this is not really a problem.
tornaria
force-pushed
the
mps-connection_graph
branch
from
abf1c80 to
8d8289b
Compare
|
I changed the PR to use A couple of tests show that a clique is added for each polynomial (but not for each connected component). |
| from sage.graphs.graph import Graph | ||
| g = Graph() | ||
| g.add_vertices(sorted(V)) |
There was a problem hiding this comment.
I do not think it is a good idea to remove this line. The point being that you want vertices to represent the variables of the underlying polynomial ring. With this removal it is not the case
sage: B.<x,y,z> = BooleanPolynomialRing()
sage: F = Sequence([B.one()])
sage: F.connection_graph() # must be a graph on three vertices and no edges
There was a problem hiding this comment.
No, that's not what this line does, since V = self.variables(). Current behaviour:
$ sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 9.8, Release Date: 2023-02-11 │
│ Using Python 3.11.3. Type "help()" for help. │
└────────────────────────────────────────────────────────────────────┘
sage: B.<x,y,z> = BooleanPolynomialRing()
sage: Sequence([x]).connection_graph()
Graph on 1 vertex
In fact, your example is currently broken:
sage: Sequence([B.one()]).connection_graph()
...
IndexError: tuple index out of range
With my patch:
sage: Sequence([B.one()]).connection_graph()
Graph on 0 vertices
I grant that these two examples should be added to the doctest. The "correct" behaviour depends on interpretation of "Return the graph which has the variables of this system as vertices...".
There was a problem hiding this comment.
I added all of these tests to document behaviour:
sage: F = Sequence([], B)
sage: F.connection_graph()
Graph on 0 vertices
sage: F = Sequence([1], B)
sage: F.connection_graph()
Graph on 0 vertices
sage: F = Sequence([x])
sage: F.connection_graph()
Graph on 1 vertex
sage: F = Sequence([x, y])
sage: F.connection_graph()
Graph on 2 vertices
There was a problem hiding this comment.
@videlec if you are satisfied, please don't forget to approve / positive_review. Thanks!
|
Documentation preview for this PR is ready! 🎉 |
As discussed in #35512, we propose an implementation of method `PolynomialSequence.connected_components()` that avoids the effective construction of the graph connecting variables that appear in a same polynomial. ### 📚 Description We use a union-find data structure to encode the connected components. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies See discussion in #35512 URL: #35518 Reported by: David Coudert Reviewer(s): David Coudert, Gonzalo Tornaría, Vincent Delecroix
Simplify the code; also, the previous code has to iterate over variables of the sequence twice (this is really bad before #35510).
This affects mainly the method
connected_components().EDIT:
Moreover, now we add a clique between the variables of each polynomial, so it agrees with the description (the code before used to add just a spanning tree of the clique -- a star).
This makes this method a little bit slower for the purposes of
connected_components()(for which adding a star is equivalent).However, #35518 will rewrite
connected_components()without usingconnection_graph()so this is not really a problem.📝 Checklist
Dependencies