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

implementation of the cluster complex #10819

Closed
stumpc5 opened this issue Feb 21, 2011 · 72 comments
Closed

implementation of the cluster complex #10819

stumpc5 opened this issue Feb 21, 2011 · 72 comments

Comments

@stumpc5
Copy link
Contributor

stumpc5 commented Feb 21, 2011

The patch contains an implementation of the cluster complex for finite types.

sage: ClusterComplex(['A',3])
Cluster complex of type ['A', 3] with 9 vertices and 14 facets

The implementation depends on its realization as a subword complex.

Depends on #20111
Depends on #20027

CC: @tscrim @stumpc5

Component: combinatorics

Keywords: cluster complex

Author: Christian Stump, Frédéric Chapoton

Branch/Commit: 3c7d5ae

Reviewer: Travis Scrimshaw

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

@fchapoton
Copy link
Contributor

comment:1

some comments :

there is a typo in "facet" at the beginning.

The examples are wrong, because they call "associahedron" which has nothing to do with the simplicial complex.

There is a "tba" in the examples section, that should be removed or replaced by a true example

You should include serious tests, like the computation of the homology of an example.

@stumpc5

This comment has been minimized.

@stumpc5
Copy link
Contributor Author

stumpc5 commented Jun 8, 2011

Dependencies: 10538,

@fchapoton
Copy link
Contributor

comment:3

Apply trac_10819-cluster_complex-cs.patch

@fchapoton
Copy link
Contributor

Changed dependencies from 10538, to #10538

@fchapoton
Copy link
Contributor

comment:5

Hi,

  1. here also, the header is not correct, you should add ticket number implementation of the cluster complex #10819.

  2. this seems to be a real error, which appears several times :

AttributeError: 'sage.matrix.matrix_integer_sparse.Matrix_integer_sparse' object has no attribute 'is_skew_symmetrizable'

@stumpc5
Copy link
Contributor Author

stumpc5 commented Jun 10, 2011

comment:6

Replying to @fchapoton:

  1. here also, the header is not correct, you should add ticket number implementation of the cluster complex #10819.

I fixed it -- now I think I know what (not why) to do in order to pass the buildbot. Thanks!

  1. this seems to be a real error, which appears several times :

There was one dependency missing, see #10298. This is like only knowing one linear extension of the ordering given by the dependencies -- so getting the dependencies is really try and error...

@stumpc5 stumpc5 added this to the sage-4.7.2 milestone Jun 11, 2011
@fchapoton
Copy link
Contributor

comment:8

Hello Christian. Here are two things that need to be corrected : 

  1. type A_n is in n+3 sided polygon

  2. formatting of doc is not correct in cartan_type

@stumpc5
Copy link
Contributor Author

stumpc5 commented Oct 5, 2011

comment:9

Replying to @fchapoton:

I really appreciate that you do some progress on these patches, thanks!

I think I will reimplement this patch using subword complexes rather than the cluster implementation. This will be much faster, and I don't expect that the cluster package will become part of main sage soon (see the dependency above). On the other hand, the subword complexes depend on my new implementation of finite reflection groups (#11187) which depends on the implementation of the universal cyclotomic field (#8327), so it will also be unlikely that it will get positively reviewed soon (and I see that all of them have rejects in the newest versions, so it will be an even longer way).

  1. type A_n is in n+3 sided polygon
  2. formatting of doc is not correct in cartan_type

I will update the patch fixing these issues.

Best, Christian

@stumpc5

This comment has been minimized.

@stumpc5
Copy link
Contributor Author

stumpc5 commented Oct 8, 2011

comment:10

I fixed the two issues suggested by Frederic - what I do not like now is that the vertices are not positive roots anymore, but on the other hand the implementation is now valid for all finite Coxeter groups and also the multi-cluster complex can be constructed.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 30, 2012

comment:11

Applying this and its dependencies to 5.0.beta11 results in a Sage install that won't start up, and hence fails every doctest in the library -- see patchbot logs.

@loefflerd loefflerd mannequin added s: needs work and removed s: needs review labels Mar 30, 2012
@stumpc5
Copy link
Contributor Author

stumpc5 commented Mar 30, 2012

comment:12

Replying to @loefflerd:

Applying this and its dependencies to 5.0.beta11 results in a Sage install that won't start up, and hence fails every doctest in the library -- see patchbot logs.

Thanks for looking at it - for now, I would prefer if we postpone this patch, since I want to make some further improvements here.

Best, Christian

@stumpc5
Copy link
Contributor Author

stumpc5 commented Jan 23, 2013

comment:13

Attachment: trac_10819-cluster_complex-cs.patch.gz

@stumpc5
Copy link
Contributor Author

stumpc5 commented Jan 23, 2013

Changed dependencies from #10538 to #11010

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

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

8878fdcadded a few tests and docs, commented 2 methods that depend on #11187

@tscrim
Copy link
Collaborator

tscrim commented Mar 7, 2016

comment:40

In a perfect world, I would just put it directly in a branch which depends on the branch which implements the reflection groups. However, I know that can be a hassle. So if it is just few lines, then I would say yes it is okay, but make sure to state clearly which ticket will change those lines. If they are entire methods, I'm more reluctant, but again, I'm not strictly opposed to it...

@stumpc5
Copy link
Contributor Author

stumpc5 commented Mar 7, 2016

comment:41

I already pushed that change, so have a look and tell me what you think concretely there...

@stumpc5
Copy link
Contributor Author

stumpc5 commented Mar 7, 2016

comment:42

It is the two methods

+    # the following methods only make sense for the implementation using #11187:
+
+    #def upper_cluster(self):
+    #def product_of_upper_cluster(self):

I might actually implement them in a much slower way now, so I guess that might be the way to go... (and then commenting out the possible speed improvement with #11187)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

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

9f6fe10added naive implementations of upper cluster and its product

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

Changed commit from 8878fdc to 9f6fe10

@stumpc5
Copy link
Contributor Author

stumpc5 commented Mar 7, 2016

comment:44

Okay, I now use a naive implementation. But I am missing

AttributeError: 'CoxeterMatrixGroup_with_category' object has no attribute 'root_to_reflection'

Is there chance to get this method ? Otherwise, I will just delete that method needing this for now. I can then get it back later, if desired.


New commits:

9f6fe10added naive implementations of upper cluster and its product

@tscrim
Copy link
Collaborator

tscrim commented Mar 7, 2016

comment:45

I think this is the best way to do things (at least because their implementation was simple).

Coxeter groups don't necessarily have all of the methods that Weyl groups have (since they are not tied to a particular root lattice). You should be able to use W.reflections()[beta] from #20027, and it should work as well for Weyl groups now that the behavior is consistent.

@tscrim
Copy link
Collaborator

tscrim commented Mar 7, 2016

comment:46

However, that's not to say we should not implement a method root_to_reflection for (finite) Coxeter groups.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

Changed commit from 9f6fe10 to e1f49c3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

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

e1f49c3added product of upper cluster

@stumpc5
Copy link
Contributor Author

stumpc5 commented Mar 7, 2016

Changed dependencies from #20111 to #20111, #20027

@stumpc5
Copy link
Contributor Author

stumpc5 commented Mar 7, 2016

New commits:

e1f49c3added product of upper cluster

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

Changed commit from e1f49c3 to f1d1dff

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

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

90b278aReversing the family for reflections.
8c9b809Making CoxeterGroup.reflections return a Family and some touchups.
2595826Removing caching of CoxeterGroup.positive_roots().
7ed4c2aMerge 7.1.beta6.
3c32075Fixing doctest failures in the thematic tutorials.
35eb71cFixing some documentation and adding note to WeylGroup.reflections.
46565ecMissed (hopefully the last) one instance of the flipped keys/values.
a93c402Merge branch 'u/stumpc5/20027' into u/stumpc5/10819
e72d0a3removed product of upper cluster for now
f1d1dffproduct is back again, though not clear why

@stumpc5
Copy link
Contributor Author

stumpc5 commented Mar 7, 2016

comment:50

(hey travis, are you in korea at the moment?)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

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

28ebb5bSome reviewer changes.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

Changed commit from f1d1dff to 28ebb5b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

Changed commit from 28ebb5b to 3c7d5ae

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2016

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

3c7d5aeMissed a trailing whitespace.

@tscrim
Copy link
Collaborator

tscrim commented Mar 7, 2016

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Mar 7, 2016

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

@tscrim
Copy link
Collaborator

tscrim commented Mar 7, 2016

comment:54

Indeed, greetings from Korea.

I pushed some changes:

  • I moved the description of the cluster complex to the class-level doc (instead of the module-level doc) so it can be visible from ClusterComplex?.
  • I updated the reference bibliographic info.
  • I added more tests to __classcall__ to make sure it covers all of the code paths/input formats.
  • I removed the verbose output from TestSuite as these can change as tests get added and it's annoying to make these trivial changes all over Sage (not that it happens that often).
  • Other misc tweaks.

If you agree with my changes, then you can set a positive review.

@vbraun
Copy link
Member

vbraun commented Mar 9, 2016

Changed branch from public/ticket/10819 to 3c7d5ae

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

5 participants