Skip to content
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

new implementation of method is_transitive with linear memory space #39221

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

dcoudert
Copy link
Contributor

The previous version of method is_transive was building the distance matrix of the digraph and so had memory usage in $O(n^2 + m)$. We change that to a version with memory usage in $O(n + m)$. In addition, this new version is faster for non-transitive digraphs.

Before

sage: D = digraphs.Circuit(4)
sage: D.is_transitive()
False
sage: %timeit D.is_transitive()
8.01 µs ± 35.7 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: D = digraphs.RandomDirectedGNP(30,.2)
sage: D.dig6_string()
']AOGb?dA?pG?S?@?OPIGG???_?O?GC??W_H?BCjaJJA?gO@?A??_?VDHG_ACCiBU?O`HaUC?kAAD@_AAJErW_G_ICCGa@?S@Oo?IC????DoCQ?Q?@?_@g?O?C?aAGK??o?cKO_W???A?G?Go?H`??Co'
sage: D.is_transitive()
False
sage: %timeit D.is_transitive()
54.3 µs ± 167 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: D = digraphs.DeBruijn(5, 2)
sage: D.is_transitive()
False
sage: %timeit D.is_transitive()
80.6 µs ± 116 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: D = digraphs.RandomDirectedGNP(20,.2).transitive_closure()
sage: D.dig6_string()
'S^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~w'
sage: D.is_transitive()
True
 sage: %timeit D.is_transitive()
56 µs ± 181 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

With this PR

sage: D = digraphs.Circuit(4)
sage: D.is_transitive()
False
sage: %timeit D.is_transitive()
6.63 µs ± 26.8 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: D =
DiGraph(']AOGb?dA?pG?S?@?OPIGG???_?O?GC??W_H?BCjaJJA?gO@?A??_?VDHG_ACCiBU?O`HaUC?kAAD@_AAJErW_G_ICCGa@?S@Oo?IC????DoCQ?Q?@?_@g?O?C?aAGK??o?cKO_W???A?G?Go?H`??Co')
sage: D.is_transitive()
False
sage: %timeit D.is_transitive()
46.7 µs ± 88.4 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: D = digraphs.DeBruijn(5, 2)
sage: D.is_transitive()
False
sage: %timeit D.is_transitive()
73.8 µs ± 116 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: D = DiGraph('S^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~z~~~^~~w')
sage: D.is_transitive()
True
sage: %timeit D.is_transitive()
58.6 µs ± 118 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Dec 29, 2024

Documentation preview for this PR (built with commit 2c64c7d; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@user202729
Copy link
Contributor

Can you just reuse some existing implementation of dfs/bfs e.g. shortest_path_all_vertices?

@dcoudert
Copy link
Contributor Author

Can you just reuse some existing implementation of dfs/bfs e.g. shortest_path_all_vertices?

We could do that, but all my trials are way slower. That's why I decided to go this way.
Before, all the complexity of the code was hidden in the call to c_distances_all_pairs.

@user202729
Copy link
Contributor

Of course it's not as much about complexity, but avoiding duplication of code.

Where does the slowness come from? both are O(n ⋅ (n + m)) right? (function call overhead? → cpdef / bfs instead of dfs? / specialized algorithm?)

@dcoudert
Copy link
Contributor Author

This new version is slightly simpler as it calls simple_BFS for the exploration part. The running time is similar. It uses one extra array of size $n$.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants