sage icon indicating copy to clipboard operation
sage copied to clipboard

switch to nauty for generating posets

Open fchapoton opened this issue 1 year ago • 8 comments

as there is now a direct method in nauty and we have added the interface previously

:memo: Checklist

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

fchapoton avatar Oct 09 '24 19:10 fchapoton

Documentation preview for this PR (built with commit 17f72fa4080c60a8a2d772ec4c21fd928c7471ed; changes) is ready! :tada: This preview will update shortly after each push to this PR.

github-actions[bot] avatar Oct 09 '24 20:10 github-actions[bot]

This is a very impressive speedup:

sage: %time n=7; sum(1 for dig in DiGraphGenerators()(n, is_poset))
CPU times: user 5.85 s, sys: 0 ns, total: 5.85 s
Wall time: 5.85 s
2045
sage: %time n=7; sum(1 for dig in DiGraphGenerators().nauty_posetg(f"{n} o"))
CPU times: user 57.3 ms, sys: 0 ns, total: 57.3 ms
Wall time: 66.7 ms
2045
sage: %time n=8; sum(1 for dig in DiGraphGenerators().nauty_posetg(f"{n} o"))
CPU times: user 395 ms, sys: 0 ns, total: 395 ms
Wall time: 399 ms
16999

mantepse avatar Oct 10 '24 10:10 mantepse

Actually, there is another issue: nauty_posetg works only for $n\leq 16$. You could use a switch to use it only in this case and use the previous (slower) method otherwise.

dcoudert avatar Oct 10 '24 10:10 dcoudert

Actually, there is another issue: nauty_posetg works only for n ≤ 16 . You could use a switch to use it only in this case and use the previous (slower) method otherwise.

Great catch!

To avoid such oversights in future (not in this pull request!), wouldn't it be better to have intermediate methods that call nauty_posetg and do the argument parsing? I am frequently wasting time looking up the precise string for nauty_geng to generate connected graphs only :-)

mantepse avatar Oct 10 '24 11:10 mantepse

I think that the issue pointed out by @dcoudert still needs fixing.

mantepse avatar Oct 10 '24 12:10 mantepse

We lose a lot of speed in the relabeling..

fchapoton avatar Oct 10 '24 13:10 fchapoton

I don't see how to avoid the topological sort. However, you can avoid a copy of the graph

-            yield FinitePoset(dig.relabel(label_dict, inplace=False))
+            yield FinitePoset(dig.relabel(label_dict, inplace=True))

note also that instead of dig.order(), you can use self._n. All graphs have the same number of vertices.

dcoudert avatar Oct 10 '24 14:10 dcoudert

merci pour les suggestions

fchapoton avatar Oct 10 '24 15:10 fchapoton