New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move reversed graph from backend to CGraph for sparse graphs #28904
Comments
comment:1
TBtw, there is a bug that leads to a crash with:
This ticket also fixes this (the method |
Commit: |
Author: Jonathan Kliem |
Branch: public/28904 |
comment:2
This probably needs to be cleaned up a lot. New commits:
|
This comment has been minimized.
This comment has been minimized.
comment:4
One needs to be careful, about taking the same set of edges I guess. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:6
Thank you for doing this, it's really needed. With these changes,
in
|
comment:7
Ticket retargeted after milestone closed |
Changed branch from public/28904 to public/28904-reb |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:10
Thanks for the comments. Replying to @dcoudert:
Fixing this makes a mistake apparent: There is a doctest in |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:12
- cdef int error = self._del_arc_label_unsafe(u, v, l, self.vertices)
- if error:
- return 1 # indicate an error
+ if self._del_arc_label_unsafe(u, v, l, self.vertices):
+ return 1 # indicate an error
I confirm that vertex deletion is much faster now. |
comment:13
There are tons of compilation warnings, as we include all of The compilation warning about wrong casting of My approach now: Remove the attribute |
comment:15
I'm happy with the current code, but it would be better to get extra opinion (e.g., from the thread https://groups.google.com/forum/#!topic/sage-devel/2eIaD3r8Vl8). |
comment:16
There remains the question, whether we should remove The advantage of removing it, is that code referring to it won't compile anymore and/or give a proper warning. Currently, if one assumes this to be initialized and uses it in cython, sage might crash hard (segmentation fault I believe). |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:18
I removed the attribute The method The method |
comment:19
Why
|
comment:20
This is independent from the purpose of this ticket, but this is effectively an issue to be fixed. |
comment:21
I updated the description of the ticket to account for the change. |
This comment has been minimized.
This comment has been minimized.
comment:22
Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date. |
comment:23
What's the status of this ticket? |
comment:24
Needs review. It's annoying to check, I guess. But it would be a nice improvements with many applications. Maybe I should request a review on sage-devel. |
comment:25
overall, it's a very nice improvement. I have only a few suggestions.
INPUT:
- u, v -- non-negative integers
+
+ - ``u, v`` -- non-negative integers Of course, we can do that in a future ticket. Not a priority here
|
Changed branch from public/28904-reb to public/28904-reb2 |
comment:27
Replying to @dcoudert:
Done.
This is being cythonized into
Done.
Done. It's a rather large commit though as I went over all the places that "caught my eye".
The last two I would leave for future tickets. But you are right. We really iterate over a structure that is already there (either over a bitset or a tree). So there is no use in wasting resources for creating this list. |
comment:28
OK to let the change from list to iterator for another ticket. LGTM. Thank you. |
Reviewer: David Coudert |
comment:29
Thanks. |
Changed branch from public/28904-reb2 to |
Currently, for sparse graphs the backend keeps a copy of the reversed graph. This makes iterating over ingoing neighbors much faster.
However,
SparseGraph
itself does not have access to this. So e.g. when deleting vertices, this results in a massive slow down as it needs to delete incoming edges.We initialize the sparse backend with the reversed structure and instead add the reversed structure directly to
SparseGraph
.Before this ticket:
With this ticket:
We fix an issue in
subdivide_edges
that would have caused a doctest to fail with the new setup:There is a doctest calling the method
subdivide_edges
withself.edges()
. As one can see from the documenation fromEdgesView
, one should not modify the graph while iterating throughEdgesView
, because it updates itself. So the following test ingeneric_graph.py
is wrong usage ofsubdivide_edges
oredges
:However, as it seems very natural to do this, we pass a tuple instead of
EdgesView
to the backend.CC: @videlec @dcoudert
Component: graph theory
Keywords: sparse graphs, backend
Author: Jonathan Kliem
Branch/Commit:
a46e717
Reviewer: David Coudert
Issue created by migration from https://trac.sagemath.org/ticket/28904
The text was updated successfully, but these errors were encountered: