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

faster generator for noncrossing partitions #27924

Closed
fchapoton opened this issue Jun 4, 2019 · 32 comments
Closed

faster generator for noncrossing partitions #27924

fchapoton opened this issue Jun 4, 2019 · 32 comments

Comments

@fchapoton
Copy link
Contributor

By not running over the full reflection group..

CC: @tscrim @stumpc5

Component: combinatorics

Author: Frédéric Chapoton, Christian Stump

Branch/Commit: 2dc91f5

Reviewer: Christian Stump, Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/27924

@fchapoton fchapoton added this to the sage-8.8 milestone Jun 4, 2019
@fchapoton
Copy link
Contributor Author

New commits:

c4c7bb4faster generation of noncrossing partitions

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/27924

@fchapoton
Copy link
Contributor Author

Commit: c4c7bb4

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 4, 2019

comment:2

I am a little uncertain whether or not to compute the reflection length in the unitary group or not.

Reflection length is the minimal number of reflections needed to write the element, in_unitary_group means the dimension of the move-space dim(w-1).

Both are equal for real groups and, in complex groups, for elements below Coxeter elements. It remains unclear to me whether or not we are allowed to use in_unitary_group here for the computation (which is in general much faster).

@fchapoton
Copy link
Contributor Author

comment:3

I tried using in_unitary_group and the timings were similar.

By the way, I cannot test with gap3, as I am not able to install Chevie.

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 4, 2019

comment:4

Well, I only implemented all this with chevie in mind and at hand. Let me do some timings before proceeding. Also, I have to think about the theoretical question how to compute the noncrossing partitions.

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 4, 2019

comment:5

There is no difference for real groups, but there is for non-real complex groups:
Using reflection lengths, we get

sage: W = ReflectionGroup([3,1,4])
sage: %time len(list(W.elements_below_coxeter_element()))
CPU times: user 8.41 s, sys: 2.36 s, total: 10.8 s
Wall time: 2min 16s
70

while using move space dimensions, we get

sage: W = ReflectionGroup([3,1,4])
sage: %time len(list(W.elements_below_coxeter_element()))
CPU times: user 2.42 s, sys: 633 ms, total: 3.05 s
Wall time: 3.38 s
70

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 4, 2019

comment:6

I believe that I convinced myself at some point that we are allowed to use move space dimension here, so
if u.reflection_length(in_unitary_group=True) + 1 == w_len instead of if u.reflection_length() + 1 == w_len, but I cannot remember the argument at the moment.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 4, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

98cdb0ftrac 27924 some details

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 4, 2019

Changed commit from c4c7bb4 to 98cdb0f

@fchapoton
Copy link
Contributor Author

comment:8

ok, here it goes. Did you check if the new algo is a strong improvement over the old one ?

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 4, 2019

comment:9

Yes it is a good improvement:

sage: W = ReflectionGroup([3,1,4])
sage: %time len(list(W.elements_below_coxeter_element()))
CPU times: user 5.41 s, sys: 1.28 s, total: 6.69 s
Wall time: 7.03 s
70

@fchapoton
Copy link
Contributor Author

comment:10

So, is this a positive review ? Patchbot is green.

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 5, 2019

Changed branch from u/chapoton/27924 to u/stumpc5/27924

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 5, 2019

comment:12

So, is this a positive review?

No, I am now clarifying the methods "elements_below_coxeter_element" and "noncrossing_partition_lattice". Have to leave now, will get back this afternoon. Let me know what you think in case you care!


New commits:

2c747ecsimplifying and stramlining noncrossing partition lattices

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 5, 2019

Changed commit from 98cdb0f to 2c747ec

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2019

Changed commit from 2c747ec to 39aec91

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

39aec91polishing ncp and doctests

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 5, 2019

Changed author from Frédéric Chapoton to Frédéric Chapoton, Christian Stump

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 5, 2019

comment:14

okay, ready for review, I guess.

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 5, 2019

Reviewer: Christian Stump

@fchapoton
Copy link
Contributor Author

comment:15
  • doc of reflection_order_ideal must be refreshed (INPUT in particular)

  • deprecation must be doctested

  • commented lines should be removed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

72103cfupdated doctests, removed commented code

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2019

Changed commit from 39aec91 to 72103cf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

002a870missed indention

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2019

Changed commit from 72103cf to 002a870

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 5, 2019

comment:18

okay, done

@fchapoton
Copy link
Contributor Author

comment:19

I have made some small changes. If you agree with them, you can set to positive.


New commits:

2dc91f5trac 27924 some details

@fchapoton
Copy link
Contributor Author

Changed branch from u/stumpc5/27924 to public/ticket/27924

@fchapoton
Copy link
Contributor Author

Changed commit from 002a870 to 2dc91f5

@stumpc5
Copy link
Contributor

stumpc5 commented Jun 6, 2019

Changed reviewer from Christian Stump to Christian Stump, Frédéric Chapoton

@vbraun
Copy link
Member

vbraun commented Jun 7, 2019

Changed branch from public/ticket/27924 to 2dc91f5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants