-
-
Notifications
You must be signed in to change notification settings - Fork 452
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
Add a method to enumerate cographs #37977
Conversation
Documentation preview for this PR (built with commit c8a1949; changes) is ready! 🎉 |
proposed - typo
looks like O(n*Mn) but should be |
Thanks fo pointing these typos. We could certainly improve the computation time (in future PR).
|
Have you re-used any code or design from Marianna Spyrakou's work? If so, her name should be in copyright notice. |
Any reason to re-implement next partition? Lexicographically next partition to a given one can be done by Sage already.
|
Unfortunately that implementation is fairly generic and slow (it goes through the list of all partitions and simply finds the current one and then gets the next). Also, the next partition here is opposite of what Sage does (lex increasing versus decreasing). |
Well, perhaps it's a good moment to improve Sage's Partitions using David's code here? (we'd have |
@dimpase, @tscrim. Concerning the enumeration of partitions, the iterator of The paper also proposed an algorithm, |
It would be nice to have that, but it doesn't do so well starting from the middle. Well, I guess setting the invariants correctly would work, but that wouldn't necessarily be how I would implement it... |
@dimpase, @tscrim I have almost all the material ready to push a PR with 4 different algorithms for generating partitions. Some algorithms generate partitions in increasing/decreasing lexicographic order and partitions are represented in ascending/descending orders. On the way I have prepared a I currently need #38020 to get a working sage installation. Can I push a PR based on that or is it better if I wait until the develop branch is fixed on my side ? |
Could you make your build with #38020 and then go back to being based off clean |
Well, I tried going switching to develop (without any further action), then back to my working branch and run |
switch to Linux :-) |
I was able to push the PR from a lux fedora server. This is #38054. |
As discussed in sagemath#37977, we implement new iterators over the partitions of an integer to get iterators generating partitions in increasing/decreasing lexicographic orders with partitions in ascending/descending orders. We also implement methods yielding the next partition in a specific ordering. ### 📝 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. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] 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#38054 Reported by: David Coudert Reviewer(s): David Coudert, Travis Scrimshaw
As discussed in sagemath#37977, we implement new iterators over the partitions of an integer to get iterators generating partitions in increasing/decreasing lexicographic orders with partitions in ascending/descending orders. We also implement methods yielding the next partition in a specific ordering. ### 📝 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. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] 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#38054 Reported by: David Coudert Reviewer(s): David Coudert, Travis Scrimshaw
May be I should add a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for these internal classes/computational files/etc. it is better to have things be public for ease in debugging, more transparency, and possible reuse. However, that is not a very strong opinion and not always applied consistency in my own code depending on what I think a user/debugger might want to see. I leave such decisions up to you here, but the current code is fine by me.
You can disregard my additional change if you want. In either case, you can set a positive review afterwards.
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Thanks for the suggestion. Let's see if it goes well. |
This is now going well, so I'm setting this PR to positive review. Thank you for the review. |
As suggested in #37964, this PR add a method to enumerate cographs of order
n
. We add a new file that could collect all methods related to cographs (enumeration, random generator, identification, etc.).The original implementation is due to Marianna Spyrakou (https://github.com/MariannaSpyrakou/Cograph_generator).
📝 Checklist
⌛ Dependencies