#20973 closed enhancement (fixed)
Cartan type Aoo
Reported by: | andrew.mathas | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.3 |
Component: | combinatorics | Keywords: | Cartan type, A infinity |
Cc: | sage-combinat, nthiery, tscrim | Merged in: | |
Authors: | Andrew Mathas | Reviewers: | Nicolas M. Thiéry, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | c1b57e6 (Commits) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
Gives a minimal implementation of the A_oo
Cartan type:
sage: CartanType(['A',oo]) ['A', oo] sage: CartanType(['A',oo]).is_irreducible() True sage: CartanType(['A',oo]).is_simply_laced() True sage: print(CartanType(['A', oo]).ascii_art()) ...---O---O---O---O---O---O---O---... -3 -2 -1 0 1 2 3
In addition, the ticket fixes the following error in CartanType
:
sage: CartanType(['A',-1]) File "<string>", line unknown ^ SyntaxError: unexpected EOF while parsing
This, and other nonsensical input, now raises a ValueError
.
See related discussion on sage-combinat.
Change History (30)
comment:1 Changed 5 years ago by
- Branch set to u/andrew.mathas/cartan_type_aoo
comment:2 Changed 5 years ago by
- Commit set to 257b283a7b30fc46cc5bae0bbfcd12037e56eb62
- Component changed from PLEASE CHANGE to combinatorics
- Description modified (diff)
- Keywords Cartan type A infinity added
- Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 5 years ago by
- Description modified (diff)
comment:4 Changed 5 years ago by
- Description modified (diff)
comment:5 Changed 5 years ago by
- Description modified (diff)
comment:6 Changed 5 years ago by
- Description modified (diff)
comment:7 Changed 5 years ago by
- Cc sage-combinat nthiery tscrim added
comment:8 Changed 5 years ago by
- Commit changed from 257b283a7b30fc46cc5bae0bbfcd12037e56eb62 to 9778f9738c6bf83c66f51ff22ba722e75e96d6ba
Branch pushed to git repo; I updated commit sha1. New commits:
9778f97 | Fixing doctest
|
comment:9 Changed 5 years ago by
- Status changed from new to needs_review
It is very rudimentary, but comments/suggestions/review welcome
comment:10 follow-up: ↓ 14 Changed 5 years ago by
Hi Andrew,
Overall, this looks good! It's a bit frustrating to have to write so many methods that are very similar to existing ones, but I did not quite see a good way to factor stuff out either.
One thing that could be done is to add a class CartanType_standard
in cartan_type.py (and inherit from it in the CartanType_standard_*
classes. At least, the inheritance from UniqueRep
and SageObject
would be written out only once.
Plugin failures:
- startup: I guess we can ignore that one
- trac: in cartan_type.py, the second quote in
trac:
20973'` should be a backquote
Some comments and suggestions:
The method description in the docstrings is not rather inconsistent. I can see the origin: the rest of the root system code that you took inspiration from is not very consistent either (mostly by my fault :-)). Still it would be good to make it more uniform.
r""" Return ... . This implements :meth:`` by returning True. EXAMPLES:: """
I would move most of the doctests from type_A_infinity.CartanType.__init__
in the class docstring, as this is useful documentation. Maybe with a bit of text interspersed explaining what this type is about (if this feels natural to you).
While you are at it, it should be easy to implement the index_set
method.
Cheers,
Nicolas
comment:11 Changed 5 years ago by
Thinking twice about it, Travis is probably right: once dynkin_diagram
will be implemented (which should boil down to returning some object such that t[i][j]
returns the right thing), there is a non trivial chance that the rest of the root system code would work with just minor glitches here and there.
But if you don't need it yourself, that's for a later ticket.
comment:12 Changed 5 years ago by
Ah: you may want to add this new type to CartanType.sample
.
comment:13 follow-up: ↓ 15 Changed 5 years ago by
I would like to future proof this by deciding how we want to differentiate between A+oo and Aoo. Unfortunately oo
is +Infinity
, so something as subtle as the difference between unsigned infinity and plus infinity might not go over so well. I don't want to hold this up (because this is a good improvement and test use-case), but I do think it is something we should discuss for a moment.
comment:14 in reply to: ↑ 10 Changed 5 years ago by
Replying to nthiery:
One thing that could be done is to add a class
CartanType_standard
in cartan_type.py (and inherit from it in theCartanType_standard_*
classes. At least, the inheritance fromUniqueRep
andSageObject
would be written out only once.
Thanks. I almost did something like this when I when I was putting this together. I agree it would be better this way, so I'll try and clean this up a little. Along similar lines, there seems to be quite a lot of code duplication between cartan_matrix
, cartan_type
, dynkin_diagram
, ... I not familiar with the nuances, differences and uses of all of this code but it would be good to clean this up down the track.
Andrew
comment:15 in reply to: ↑ 13 Changed 5 years ago by
Replying to tscrim:
I would like to future proof this by deciding how we want to differentiate between A+oo and Aoo. Unfortunately
oo
is+Infinity
, so something as subtle as the difference between unsigned infinity and plus infinity might not go over so well. I don't want to hold this up (because this is a good improvement and test use-case), but I do think it is something we should discuss for a moment.
As you suggested implementing both the oo
and +oo
cases I thought about this and got stuck on precisely the problem that oo == +Infinity
. If anyone has a good idea as to how this should work in terms of syntax I am happy to implement it. It is unfortunate that we can't use:
sage: CartanType(['A', oo]) sage: CartanType(['A',+oo])
but, as you say, this won't work.
Another direction for relatively easy generalisation is B_oo
etc.
I have a related question concerning cartan_matrix
, and possibly dynkin_diagram
. I was surprised that matrices indexed by Z
and N
are not implemented in sage and, similarly, that graphs with infinite vertex sets are not supported. It would be very easy to implement a fake Cartan matrix, say C
, that given two integers makes C[i,j]
, or C[i][j]
, return the corresponding entry of the Cartan matrix. Is some one able to tell me what functionality such a matrix would need in order to be useful to the root system code? Similarly, implementing a fake Dynkin diagram class would be straightforward as long as I knew what methods I have to implement.
comment:16 Changed 5 years ago by
Perhaps it would be a bit strange to do ['A', oo, 1]
for Z as it is more like A1(1) and it differs from the usual limits of the other finite types.
It's been a while since I looked deeply into the Dynkin diagram, Cartan type/matrix code. They all inherit from CartanType_abstract
, but they usually need to direct to the correct representative object to work correctly. One of the good things is that we can add in functionality in layers (i.e., we don't need to do things all at once to make improvements). So a simple class with basic functionality can be a good starting point (on another ticket); this is what I did with Coxeter types.
comment:17 Changed 5 years ago by
Just a brief note for now: for the syntax for the various types of infinity, what about using
sage: CartanType(['A',\NN]) sage: CartanType(['A',\ZZ])
The implementation could be generic and handle both, and even more (e.g. if someone would want to use positive integers as index set, or some parabolic subtype). We could even imagine sharing some stuff with the finite case, but that's probably overdesign.
CartanType(['A',oo])
could be set as an alias to either of them. I would tend to use \ZZ
, as the other is a parabolic subtype, but I don't know the field enough to know which one would be the natural default for a mathematician there.
comment:18 Changed 5 years ago by
bad syntax for trac link, see patchbot report
comment:19 Changed 5 years ago by
- Commit changed from 9778f9738c6bf83c66f51ff22ba722e75e96d6ba to 3c5370673410e49fc0acbff86d1ba443c72a44a8
Branch pushed to git repo; I updated commit sha1. New commits:
3c53706 | Fixing trac link
|
comment:20 follow-up: ↓ 21 Changed 5 years ago by
I have addressed all of the comments above. Given the ambiguity between oo
and +oo
I have used NN
and ZZ
throughout, although oo
is a synonym for ZZ
:
sage: CartanType(['A', NN]) ['A', NN] sage: print(CartanType(['A', NN]).ascii_art()) O---O---O---O---O---O---O---.. 0 1 2 3 sage: CartanType(['A', ZZ]) ['A', ZZ] sage: print(CartanType(['A', ZZ]).ascii_art()) ..---O---O---O---O---O---O---O---.. -3 -2 -1 0 1 2 3
There is now a CartanType_standard
class in cartan_type.py
but, to be honest, it does not save a great deal. I played round a little to try and rationalise the code but everything seems very tangled because small changes in one place cause errors elsewhere. I think that if the code is to the cleaned up then it needs to be done by some one who is more familiar with it and its' applications in sage.
comment:21 in reply to: ↑ 20 Changed 5 years ago by
Replying to andrew.mathas:
I have addressed all of the comments above. Given the ambiguity between
oo
and+oo
I have usedNN
andZZ
throughout, althoughoo
is a synonym forZZ
:
That is exactly what I was just going to suggest this morning. :P I think this is the best way forward.
There is now a
CartanType_standard
class incartan_type.py
but, to be honest, it does not save a great deal. I played round a little to try and rationalise the code but everything seems very tangled because small changes in one place cause errors elsewhere. I think that if the code is to the cleaned up then it needs to be done by some one who is more familiar with it and its' applications in sage.
Yea, I remember them being fairly interconnected, sometimes in subtle ways. I remember fighting some things when I was trying to implement hyperbolic types (which I don't know when I will get back to this).
The code works with 7.3.beta7 but it will almost certainly not merge on the next release on the develop branch given trac:18555.
If #18555 really does cause a conflict, which I agree it probably does, then let's have a preemptive strike and base this over #18555.
comment:22 Changed 5 years ago by
Also, this sentence is vague:
In sage `oo` is the same as `+Infinity` so `NN` and `ZZ` are used to differentiate between the `A_{+\infty}` and `A_{\infty}` root systems.
In particular, what does oo
do? I would write the entire docstring as:
class CartanType(CartanType_standard, CartanType_simple): r""" Define the Cartan type `A_{\infty}`. We use `NN` and `ZZ` to explicitly differentiate between the `A_{+\infty}` and `A_{\infty}` root systems. While `oo` is the same as `+Infinity` in Sage, it is used as an alias for `ZZ`. """
I would also move all of the EXAMPLES::
from the __init__
into the class-level doc above (but leave the ones in TESTS::
in __init__
).
comment:23 Changed 5 years ago by
- Commit changed from 3c5370673410e49fc0acbff86d1ba443c72a44a8 to c600b3c2a3cc4f7c66968cca9ce6589d0d878992
comment:24 Changed 5 years ago by
- Branch changed from u/andrew.mathas/cartan_type_aoo to public/combinat/root_system/type_A_infinity-20973
- Commit changed from c600b3c2a3cc4f7c66968cca9ce6589d0d878992 to 9cd615d8a61c85d294a2812ed0826a656232a215
- Reviewers set to Nicolas Thiéry, Travis Scrimshaw
Thanks for the changes. I made a few minor reviewer changes. If you're okay with them, then you can set a positive review.
New commits:
9cd615d | Some small reviewer changes.
|
comment:25 Changed 5 years ago by
- Status changed from needs_review to positive_review
Thanks Travis. Yes, I am happy with your changes.
comment:26 Changed 5 years ago by
- Status changed from positive_review to needs_work
sage -t --long src/sage/algebras/nil_coxeter_algebra.py # 1 doctest failed sage -t --long src/sage/combinat/root_system/cartan_type.py # 1 doctest failed sage -t --long src/sage/combinat/root_system/dynkin_diagram.py # 1 doctest failed
comment:27 Changed 5 years ago by
- Commit changed from 9cd615d8a61c85d294a2812ed0826a656232a215 to c1b57e673a66cb92d92acefa2cd6f86b236ca7f9
Branch pushed to git repo; I updated commit sha1. New commits:
c1b57e6 | Fixing broken doctests
|
comment:28 Changed 5 years ago by
- Status changed from needs_work to positive_review
Sorry! All fixed.
comment:29 Changed 5 years ago by
- Branch changed from public/combinat/root_system/type_A_infinity-20973 to c1b57e673a66cb92d92acefa2cd6f86b236ca7f9
- Resolution set to fixed
- Status changed from positive_review to closed
comment:30 Changed 4 years ago by
- Commit c1b57e673a66cb92d92acefa2cd6f86b236ca7f9 deleted
- Reviewers changed from Nicolas Thiéry, Travis Scrimshaw to Nicolas M. Thiéry, Travis Scrimshaw
New commits:
Basic functionality for Aoo
Minimal implementation of Cartan type Aoo