Create a class for Coxeter matrices and types

[tscrim]

This is a partial step towards #16126, but carries the necessary information to speed up #16630 when the input is given as a finite !Cartan/Coxeter type.

It is possible to input numbers c<= -1 in the matrix to represent an infinite label, but with a different value in the bilinear form of the Tits representation.

The current implementation recognizes finite and affine types

Possible follow-up: -create an abstract class for Coxeter matrices and Cartan matrices? -Maybe change the name of CoxeterType.py could to type_coxeter.py -Recognize more types

Depends on #17990 Depends on #18152 Depends on #18743

CC: @sagetrac-sage-combinat @darijgr @fchapoton @jplab @sagetrac-vripoll @sagetrac-jmichel @nthiery @kevindilks

Component: group theory

Keywords: Coxeter groups, matrices, types, days64

Author: Travis Scrimshaw, Jean-Philippe Labbé

Branch: 5d188d4

Reviewer: Jean-Philippe Labbé, Travis Scrimshaw

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

226f47a

Making KR crystals more robust against relabeling.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 4ca4345 to 226f47a

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 226f47a to fe2e01f

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

fe2e01f

Fixed cluster seed not expicitly specifying an indexing set.

[tscrim]

comment:74

The fix was that the cluster seed code was assuming the indexing set of a Cartan matrix constructed from a matrix that was of finite type had the standard finite type indexing set of [1, 2, ..., n]. I fixed this by explicitly specifying this to be the index set of the Cartan matrix.

This brings up a point that might need a discussion: Should the index set of a matrix representing a finite type Cartan matrix be by default 1-based or 0-based? The argument for being 1-based is above, a natural assumption on index sets of finite type. Why we should have 0-based is consistency with the rest for default labellings and it agrees with the indexing set of the given matrix (and everything in python is 0-based). Thoughts?

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from fe2e01f to e659186

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

e659186

Some pyflakes cleanup.

[tscrim]

comment:76

This should at least get rid of the startup imports of the graph generators stuff.

[jplab]

comment:77

Replying to @tscrim:

The fix was that the cluster seed code was assuming the indexing set of a Cartan matrix constructed from a matrix that was of finite type had the standard finite type indexing set of [1, 2, ..., n]. I fixed this by explicitly specifying this to be the index set of the Cartan matrix.

Is my last edition to make test pass consistent with this change?

This brings up a point that might need a discussion: Should the index set of a matrix representing a finite type Cartan matrix be by default 1-based or 0-based? The argument for being 1-based is above, a natural assumption on index sets of finite type. Why we should have 0-based is consistency with the rest for default labellings and it agrees with the indexing set of the given matrix (and everything in python is 0-based). Thoughts?

I would say 1-based for finite type since they probably have a labeling hardcoded when dealing with them through Cartan matrices and Coxeter Types.

I would say 0-based whenever there is no labels specified and it comes through a CoxeterMatrix type, eventhough the type is finite.

But this probably causes trouble?

In a sense, having a "relabel" method somehow means that we have a canonical way of doing and then whenever it is not that one, it means that it was relabeled.

Does that make sense?

[tscrim]

comment:78

Replying to @jplab:

Replying to @tscrim:

The fix was that the cluster seed code was assuming the indexing set of a Cartan matrix constructed from a matrix that was of finite type had the standard finite type indexing set of [1, 2, ..., n]. I fixed this by explicitly specifying this to be the index set of the Cartan matrix.

Is my last edition to make test pass consistent with this change?

One of the trivial doctest failures because of subtype actually was the one indicating the problem, but that was because I wasn't passing the indexing set as I should have been. We should check for CoxeterMatrix and CoxeterType that labelings work as they should.

This brings up a point that might need a discussion: Should the index set of a matrix representing a finite type Cartan matrix be by default 1-based or 0-based? The argument for being 1-based is above, a natural assumption on index sets of finite type. Why we should have 0-based is consistency with the rest for default labellings and it agrees with the indexing set of the given matrix (and everything in python is 0-based). Thoughts?

I would say 1-based for finite type since they probably have a labeling hardcoded when dealing with them through Cartan matrices and Coxeter Types.

I would say 0-based whenever there is no labels specified and it comes through a CoxeterMatrix type, eventhough the type is finite.

But this probably causes trouble?

I was talking about when no labels are given, and it sounds like we are in agreement. However there is some code that doesn't use the indexing set, but instead a range under the assumption that the indexing set is (1, 2, ..., n). I'm still partially debating whether or not to change the cluster seed code as well to help make it robust (although it is somewhat moot because we now specify the indexing set).

In short, it causes trouble for people's code that had made an assumption about the indexing set before.

In a sense, having a "relabel" method somehow means that we have a canonical way of doing and then whenever it is not that one, it means that it was relabeled.

Does that make sense?

Yes it makes sense. In a way, I agree with you. However, we must define a canonical way of labeling in order to implement the database of Cartan types. Also we can relabel relabeled types.

[nthiery]

comment:79

For whatever it's worth, the strategy I followed in the root system code was to never make any assumption on the indexing set I.

For Cartan matrices, the situation is similar with that of matrices of module morphisms. Ideally they would be indexed by I; however we don't really have yet a good matrix class supporting arbitrary indexing (or do we? Panda's DataFrame class [1] could be an interesting starting point!). That's in particular why I avoided using the Cartan matrix as much as possible in computations, preferring instead the Dynkin diagram.

Until we have a class for matrices with arbitrary indexing, I believe we should stick to what has been done so far: index the matrix by 0,...,|I|-1 with row/column i corresponding to the i-th element of the indexing set I (following Python's convention: i=0 gives the first element).

Cheers, Nicolas

[1] http://www.gregreda.com/2013/10/26/working-with-pandas-dataframes/

[tscrim]

comment:80

Replying to @nthiery:

Until we have a class for matrices with arbitrary indexing, I believe we should stick to what has been done so far: index the matrix by 0,...,|I|-1 with row/column i corresponding to the i-th element of the indexing set I (following Python's convention: i=0 gives the first element).

So my takeaway from what you're saying is that you also agree that the default indexing set for Cartan matrices should be 0, 1, ..., |I|-1. Is that correct? We aren't changing the actual __getitem__ in this ticket (which always uses 0, 1, ...., |I|-1). The failures in cluster seed was caused by the root system having the "wrong" indexing set.

[jplab]

comment:81

All tests now pass on my sage-6.9 version (root_system, rigged_configuration, cluster_algebra_quiver, crystals folders)

Now, what is this startup module error in the patchbot??

[tscrim]

comment:82

Replying to @jplab:

All tests now pass on my sage-6.9 version (root_system, rigged_configuration, cluster_algebra_quiver, crystals folders)

That's good.

Now, what is this startup module error in the patchbot??

It is because we have an extra module (the one for Coxeter matices) that gets imported when Sage starts. This is there as a very mild warning to try to not increase statup time, but it's somewhat of a necessary (very small) evil for us to do here.

[jplab]

comment:83

As all tests are passed and the labeling issues were solved, I set the ticket to positive review.

[tscrim]

comment:84

Thank you for your work on this.

[vbraun]

comment:85

PDF docs don't build

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

6f8a932	Merge branch 'develop' into public/combinat/coxeter_matrices-17798
a44d071	Merge branch 'develop' into public/combinat/coxeter_matrices-17798
5d188d4	Fixing pdf build.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from e659186 to 5d188d4

[vbraun]

Changed branch from public/combinat/coxeter_matrices-17798 to 5d188d4

[seblabbe]

comment:89

Replying to @jplab:

Now, what is this startup module error in the patchbot??

It is because the new imports in the sage namespaces done in the file # file /src/sage/combinat/root_system/all.py :

from coxeter_matrix import CoxeterMatrix
from coxeter_type import CoxeterType

which automatically loads all this from src/sage/combinat/root_system/coxeter_matrix.py file:

+from sage.misc.cachefunc import cached_method
+from sage.matrix.constructor import matrix
+from sage.matrix.matrix_space import MatrixSpace
+from sage.misc.classcall_metaclass import ClasscallMetaclass, typecall
+from sage.matrix.matrix_generic_dense import Matrix_generic_dense
+from sage.graphs.graph import Graph
+from sage.rings.all import ZZ, QQ, RR
+from sage.rings.infinity import infinity
+from sage.combinat.root_system.cartan_type import CartanType
+from sage.combinat.root_system.coxeter_type import CoxeterType

and this from src/sage/combinat/root_system/coxeter_type.py file:

+from sage.misc.abstract_method import abstract_method
+from sage.misc.cachefunc import cached_method
+from sage.misc.classcall_metaclass import ClasscallMetaclass
+from sage.combinat.root_system.cartan_type import CartanType
+from sage.matrix.all import MatrixSpace
+from sage.symbolic.ring import SR
+from sage.structure.unique_representation import UniqueRepresentation
+from sage.structure.sage_object import SageObject

which explains why the patchbot was saying that Sage now takes 6 seconds instead of 5 to start. You may think about the following questions:

• Do all of these imports really need to be done at the top level of those two modules?
• Do you really need to import CoxeterMatrix and CoxeterType in the Sage namespace? I mean, it is usefull for you, for me. But, maybe that 99% of people will not use those two things while using Sage. So I think you do not have to import them in the global Sage namespace.

Anyhow, since this ticket is closed, this discussion should be moved to another ticket if it is worthwhile to continue that discussion.

[seblabbe]

Changed commit from 5d188d4 to none

[tscrim]

comment:90

You are not being fair. All of those are already imported, so they contribute so marginally to startup time that they are essentially non-existent. This can be seen by the startup time plugin which says there might have been a 1.6 millisecond increase in startup time out of 2.6 second average startup.

Also, how many things in the Sage namespace are really useful to most users of Sage? Most of the things that are there are not useful to 99% of the users. So let's advocate removing them from the global namespace, and then nothing except basic calculus will be easily discoverable.

I agree that we do need to be somewhat careful about startup time. However to do that, we need better resolution of lazy imports and large chunks of combinat to be changed to be lazily imported (see #15293). But let us keep this in perspective.

[seblabbe]

comment:91

Replying to @tscrim:

This can be seen by the startup time plugin which says there might have been a 1.6 millisecond increase in startup time out of 2.6 second average startup.

Sorry, I think the 5->6 seconds was only the time being spent by the plugin itself. Where do you read the "1.6 ms" information?

[tscrim]

comment:92

Replying to @seblabbe:

Replying to @tscrim:

This can be seen by the startup time plugin which says there might have been a 1.6 millisecond increase in startup time out of 2.6 second average startup.

Sorry, I think the 5->6 seconds was only the time being spent by the plugin itself. Where do you read the "1.6 ms" information?

See the plugins.startup_time link in the patchbot report.