Refactor degree sequence functions #41188
Conversation
Refactor degree sequence generation to use generators, reducing memory usage.
|
Can you help me check if the result is true? Thank you @maxale |
|
These changes look good to me, but with respect to refactoring, I'd also suggest to change |
The result is right. But it takes 11 minutes. Is it normal? @maxale |
Updated degree sequences representation to use tuples instead of lists for consistency and improved performance.
I have used tuple instaed of list |
| for N > i >= 0: | ||
| for 0 <= j < seq[i]: | ||
| s.append(i) |
There was a problem hiding this comment.
This is so much faster than the python equivalent, it's still surprising me.
There was a problem hiding this comment.
Thank you very much
There was a problem hiding this comment.
@mantepse I tested the version using range() and they have the exact same performance (as long as there are cdef of the loop variables), cython is good at optimization nowadays. In fact it appears to be preferred style to use range() just like in Python instead of cython's for loop style (see some of @fchapoton 's pull requests)
There was a problem hiding this comment.
The inner loop yes, but the outer seems much slower to me??? Maybe I did it wrong?
There was a problem hiding this comment.
Shouldn't the inner loop with s.append(i) be better replaced with s.extend(i for _ in range(seq[i])) ?
There was a problem hiding this comment.
Actually both loops can be done inside a single call to s.extend() - that is:
s.extend(i for i in range(N-1, -1, -1) for _ in range(seq[i]))
There was a problem hiding this comment.
Yes, but unfortunately this is slower. Also, a8ce96d is a slowdown, but it is less significant than I thought.
|
Maybe it would make sense to implement a |
|
As suggestions I would like to use range instead of this loop |
I don't think we'd usually consider this a breaking change. I don't think we have a clear policy on this. |
|
okay. cf. #41038 where |
|
Do we really want to include the last commit (which is a slowdown)? |
If It does not be slower much than before, I think it is acceptable. @user202729 |
Yes, but is there any good reason to do it? The other loops in the same file are consistently done with the same syntax, and I don't see any advantage of imitating python style in this particular case. |
|
Maybe I revert this |
I have done this. Maybe revert is much faster |
sagemathgh-41188: Refactor degree sequence functions Refactor degree sequence generation to use generators, reducing memory usage. Fix sagemath#41187 <!-- ^ 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". --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] 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 <!-- 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#41188 Reported by: Chenxin Zhong Reviewer(s): Chenxin Zhong, Martin Rubey, Max Alekseyev, user202729
sagemathgh-41188: Refactor degree sequence functions Refactor degree sequence generation to use generators, reducing memory usage. Fix sagemath#41187 <!-- ^ 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". --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] 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 <!-- 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#41188 Reported by: Chenxin Zhong Reviewer(s): Chenxin Zhong, Martin Rubey, Max Alekseyev, user202729
5 participants
Refactor degree sequence generation to use generators, reducing memory usage. Fix #41187
📝 Checklist
⌛ Dependencies