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

Add a method to enumerate cographs #37977

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

Conversation

dcoudert
Copy link
Contributor

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

  • 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 May 10, 2024

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

@dimpase
Copy link
Member

dimpase commented May 12, 2024

This method implements the generator of all cographs of order proposd in [JPD2018].

proposed - typo

[...] Hence, the overall complexity of the algorithm is

looks like O(n*Mn) but should be $O(n M_n)$

@dcoudert
Copy link
Contributor Author

Thanks fo pointing these typos.

We could certainly improve the computation time (in future PR).

n	M_n	time
2	2	0.0
3	4	0.0
4	10	0.0
5	24	0.001
6	66	0.002
7	180	0.006
8	522	0.016
9	1532	0.046
10	4624	0.139
11	14136	0.484
12	43930	1.746
13	137908	5.968
14	437502	20.997
15	1399068	73.866

@dimpase
Copy link
Member

dimpase commented May 13, 2024

Have you re-used any code or design from Marianna Spyrakou's work? If so, her name should be in copyright notice.

@dimpase
Copy link
Member

dimpase commented May 13, 2024

Any reason to re-implement next partition? Lexicographically next partition to a given one can be done by Sage already.
e.g.

sage: m=Partitions(8)
sage: m.next([8])
[7, 1]
sage: m.next(m.next([8]))
[6, 2]
sage: m.next(m.next(m.next([8])))
[6, 1, 1]

@tscrim
Copy link
Collaborator

tscrim commented May 14, 2024

Any reason to re-implement next partition? Lexicographically next partition to a given one can be done by Sage already. e.g.

sage: m=Partitions(8)
sage: m.next([8])
[7, 1]
sage: m.next(m.next([8]))
[6, 2]
sage: m.next(m.next(m.next([8])))
[6, 1, 1]

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).

@dimpase
Copy link
Member

dimpase commented May 14, 2024

Any reason to re-implement next partition? Lexicographically next partition to a given one can be done by Sage already. e.g.

sage: m=Partitions(8)
sage: m.next([8])
[7, 1]
sage: m.next(m.next([8]))
[6, 2]
sage: m.next(m.next(m.next([8])))
[6, 1, 1]

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 .previous() for what's needed here, and .next() for the opposite direction move)
Having multiple implementations of the same thing isn't good design.

@dcoudert
Copy link
Contributor Author

@dimpase : Yes, I must add Marianna Spyrakou in the copyright

@dimpase, @tscrim: I agree that it's a good idea to improve the code of next and to add method previous.

I cannot work on that yet (broken sage since last beta), but will contribute asap.

@tscrim
Copy link
Collaborator

tscrim commented May 15, 2024

@dimpase I agree it is not good design. What I am saying here is what @dcoudert is providing isn't duplicated. However, moving it to a prev(ious) method of partitions would be good.

@dcoudert
Copy link
Contributor Author

@dimpase, @tscrim. Concerning the enumeration of partitions, the iterator of Partitions is based on algorithm ZS1 of https://static.aminer.org/pdf/PDF/000/289/332/counting_and_generating_integer_partitions_in_parallel.pdf. It goes in anti-lexicographic.

The paper also proposed an algorithm, ZS2 for enumerating partitions in lexicographic order. We will certainly have to implement it.

@tscrim
Copy link
Collaborator

tscrim commented May 18, 2024

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...

@dcoudert
Copy link
Contributor Author

@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 next function based on each generator. So users should find the method they need.

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 ?

@tscrim
Copy link
Collaborator

tscrim commented May 20, 2024

Could you make your build with #38020 and then go back to being based off clean develop to do the development? In particular, without running sage -b and/or make? Does even running sage -b cause problems? I had a similar issue in the past, but it only would have appeared when running make with package building.

@dcoudert
Copy link
Contributor Author

Could you make your build with #38020 and then go back to being based off clean develop to do the development? In particular, without running sage -b and/or make? Does even running sage -b cause problems? I had a similar issue in the past, but it only would have appeared when running make with package building.

Well, I tried going switching to develop (without any further action), then back to my working branch and run sage -b. It launches a huge compilation. So I will wait for a working develop branch.

@dimpase
Copy link
Member

dimpase commented May 21, 2024

switch to Linux :-)

@dcoudert
Copy link
Contributor Author

switch to Linux :-)

I was able to push the PR from a lux fedora server. This is #38054.

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.

None yet

3 participants