Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#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 andrew.mathas)

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 3 years ago by andrew.mathas

  • Branch set to u/andrew.mathas/cartan_type_aoo

comment:2 Changed 3 years ago by andrew.mathas

  • Authors set to Andrew Mathas
  • 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

New commits:

c52edd1Basic functionality for Aoo
257b283Minimal implementation of Cartan type Aoo

comment:3 Changed 3 years ago by andrew.mathas

  • Description modified (diff)

comment:4 Changed 3 years ago by andrew.mathas

  • Description modified (diff)

comment:5 Changed 3 years ago by andrew.mathas

  • Description modified (diff)

comment:6 Changed 3 years ago by andrew.mathas

  • Description modified (diff)

comment:7 Changed 3 years ago by tscrim

  • Cc sage-combinat nthiery tscrim added

comment:8 Changed 3 years ago by git

  • Commit changed from 257b283a7b30fc46cc5bae0bbfcd12037e56eb62 to 9778f9738c6bf83c66f51ff22ba722e75e96d6ba

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

9778f97Fixing doctest

comment:9 Changed 3 years ago by andrew.mathas

  • Status changed from new to needs_review

It is very rudimentary, but comments/suggestions/review welcome

comment:10 follow-up: Changed 3 years ago by nthiery

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 3 years ago by nthiery

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 3 years ago by nthiery

Ah: you may want to add this new type to CartanType.sample.

comment:13 follow-up: Changed 3 years ago by 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.

comment:14 in reply to: ↑ 10 Changed 3 years ago by andrew.mathas

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 the CartanType_standard_* classes. At least, the inheritance from UniqueRep and SageObject 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 3 years ago by andrew.mathas

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.

Last edited 3 years ago by andrew.mathas (previous) (diff)

comment:16 Changed 3 years ago by tscrim

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 3 years ago by nthiery

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.

Last edited 3 years ago by nthiery (previous) (diff)

comment:18 Changed 3 years ago by chapoton

bad syntax for trac link, see patchbot report

comment:19 Changed 3 years ago by git

  • Commit changed from 9778f9738c6bf83c66f51ff22ba722e75e96d6ba to 3c5370673410e49fc0acbff86d1ba443c72a44a8

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

3c53706Fixing trac link

comment:20 follow-up: Changed 3 years ago by andrew.mathas

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.

Version 0, edited 3 years ago by andrew.mathas (next)

comment:21 in reply to: ↑ 20 Changed 3 years ago by tscrim

Replying to andrew.mathas:

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:

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

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 3 years ago by tscrim

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 3 years ago by git

  • Commit changed from 3c5370673410e49fc0acbff86d1ba443c72a44a8 to c600b3c2a3cc4f7c66968cca9ce6589d0d878992

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

5b02f0dMerge branch 'develop' into t/20973/cartan_type_aoo
c600b3cUpdating doc-string for A_oo

comment:24 Changed 3 years ago by tscrim

  • 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:

9cd615dSome small reviewer changes.

comment:25 Changed 3 years ago by andrew.mathas

  • Status changed from needs_review to positive_review

Thanks Travis. Yes, I am happy with your changes.

comment:26 Changed 3 years ago by vbraun

  • 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 3 years ago by git

  • Commit changed from 9cd615d8a61c85d294a2812ed0826a656232a215 to c1b57e673a66cb92d92acefa2cd6f86b236ca7f9

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

c1b57e6Fixing broken doctests

comment:28 Changed 3 years ago by andrew.mathas

  • Status changed from needs_work to positive_review

Sorry! All fixed.

comment:29 Changed 3 years ago by vbraun

  • 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 3 years ago by jdemeyer

  • Commit c1b57e673a66cb92d92acefa2cd6f86b236ca7f9 deleted
  • Reviewers changed from Nicolas Thiéry, Travis Scrimshaw to Nicolas M. Thiéry, Travis Scrimshaw
Note: See TracTickets for help on using tickets.