faster method __mul__ for graphs
#39300
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Documentation preview for this PR (built with commit 8e82bfc; changes) is ready! 🎉 |
|
Joli. Tu peux stp en profiter pour ajouter un doctest pour la ligne "raise" quand on multiplie par autre chose ? |
|
et voilà ! |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jan 9, 2025
Instead of $n-1$ additions of graphs, we use a logarithmic number of
additions of graphs to compute $G * n$.
Before
```sage
sage: def test(G):
....: for i in [1, 2, 3, 4, 5, 10, 15, 20, 50, 100]:
....: t = walltime()
....: for _ in range(10):
....: H = G * i
....: t = walltime() - t
....: print(f"{i}\t {round(t/10, 5)}")
sage: test(Graph([(0, 1)]))
1 3e-05
2 0.00011
3 0.00016
4 0.00024
5 0.00032
10 0.00072
15 0.00108
20 0.00139
50 0.00437
100 0.01272
sage: test(graphs.PetersenGraph())
1 5e-05
2 0.00017
3 0.00028
4 0.00042
5 0.00055
10 0.00148
15 0.0024
20 0.00343
50 0.01802
100 0.07057
```
Now
```sage
sage: test(Graph([(0, 1)]))
1 3e-05
2 0.0001
3 0.00018
4 0.00018
5 0.00025
10 0.00035
15 0.00053
20 0.00043
50 0.00083
100 0.00118
sage: test(graphs.PetersenGraph())
1 4e-05
2 0.00015
3 0.00028
4 0.00026
5 0.00041
10 0.00066
15 0.00117
20 0.00094
50 0.0024
100 0.00448
```
### 📝 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.
- [ ] 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
<!-- 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#39300
Reported by: David Coudert
Reviewer(s): Frédéric Chapoton
|
I found way faster method, avoiding repeated calls to sage: test(Graph([(0, 1)]))
1 4e-05
2 3e-05
3 2e-05
4 2e-05
5 2e-05
10 3e-05
15 3e-05
20 4e-05
50 7e-05
100 0.0001
sage: test(graphs.PetersenGraph())
1 5e-05
2 5e-05
3 5e-05
4 5e-05
5 6e-05
10 9e-05
15 0.00011
20 0.00015
50 0.00034
100 0.00061 |
|
hmm, not sure what Volker will think about that, as he already pushed the branch.... |
|
outch, really sorry about that. Let me know what I should do. |
|
I have no idea. |
|
Probably best just to do a new PR for the additional change. |
|
Sure I will do a new PR for the changes. I hope I have not created too many troubles when pushing the last commit on this branch. |
|
It shouldn't matter. Volker would have pulled by the commit (i.e., where the branch was previously pointing to). Although you will likely just have to push to a new branch to create the followup PR. However, there shouldn't be any issues afterwards. |
|
This has not been added to the last beta. Should we set this PR back to positive review ? or is it better to close it an open a new one ? |
|
back to positive |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jan 19, 2025
Instead of $n-1$ additions of graphs, we use a logarithmic number of
additions of graphs to compute $G * n$.
Before
```sage
sage: def test(G):
....: for i in [1, 2, 3, 4, 5, 10, 15, 20, 50, 100]:
....: t = walltime()
....: for _ in range(10):
....: H = G * i
....: t = walltime() - t
....: print(f"{i}\t {round(t/10, 5)}")
sage: test(Graph([(0, 1)]))
1 3e-05
2 0.00011
3 0.00016
4 0.00024
5 0.00032
10 0.00072
15 0.00108
20 0.00139
50 0.00437
100 0.01272
sage: test(graphs.PetersenGraph())
1 5e-05
2 0.00017
3 0.00028
4 0.00042
5 0.00055
10 0.00148
15 0.0024
20 0.00343
50 0.01802
100 0.07057
```
Now
```sage
sage: test(Graph([(0, 1)]))
1 3e-05
2 0.0001
3 0.00018
4 0.00018
5 0.00025
10 0.00035
15 0.00053
20 0.00043
50 0.00083
100 0.00118
sage: test(graphs.PetersenGraph())
1 4e-05
2 0.00015
3 0.00028
4 0.00026
5 0.00041
10 0.00066
15 0.00117
20 0.00094
50 0.0024
100 0.00448
```
### 📝 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.
- [ ] 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
<!-- 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#39300
Reported by: David Coudert
Reviewer(s): Frédéric Chapoton
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of$n-1$ additions of graphs, we use a logarithmic number of additions of graphs to compute $G * n$ .
Before
Now
📝 Checklist
⌛ Dependencies