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

Cartan type Aoo #20973

Closed
AndrewMathas opened this issue Jul 7, 2016 · 41 comments
Closed

Cartan type Aoo #20973

AndrewMathas opened this issue Jul 7, 2016 · 41 comments

Comments

@AndrewMathas
Copy link
Member

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.

CC: @sagetrac-sage-combinat @nthiery @tscrim

Component: combinatorics

Keywords: Cartan type, A infinity

Author: Andrew Mathas

Branch: c1b57e6

Reviewer: Nicolas M. Thiéry, Travis Scrimshaw

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

@AndrewMathas AndrewMathas added this to the sage-7.3 milestone Jul 7, 2016
@AndrewMathas
Copy link
Member Author

Branch: u/andrew.mathas/cartan_type_aoo

@AndrewMathas

This comment has been minimized.

@AndrewMathas
Copy link
Member Author

Commit: 257b283

@AndrewMathas
Copy link
Member Author

Author: Andrew Mathas

@AndrewMathas
Copy link
Member Author

New commits:

c52edd1Basic functionality for Aoo
257b283Minimal implementation of Cartan type Aoo

@AndrewMathas
Copy link
Member Author

Changed keywords from none to Cartan type, A infinity

@AndrewMathas

This comment has been minimized.

@AndrewMathas

This comment has been minimized.

@AndrewMathas

This comment has been minimized.

@AndrewMathas

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2016

Changed commit from 257b283 to 9778f97

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 7, 2016

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

9778f97Fixing doctest

@AndrewMathas
Copy link
Member Author

comment:9

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

@nthiery
Copy link
Contributor

nthiery commented Jul 10, 2016

comment:10

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

@nthiery
Copy link
Contributor

nthiery commented Jul 10, 2016

comment:11

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.

@nthiery
Copy link
Contributor

nthiery commented Jul 10, 2016

comment:12

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

@tscrim
Copy link
Collaborator

tscrim commented Jul 10, 2016

comment:13

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.

@AndrewMathas
Copy link
Member Author

comment:14

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

@AndrewMathas
Copy link
Member Author

comment:15

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.

@tscrim
Copy link
Collaborator

tscrim commented Jul 10, 2016

comment:16

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.

@nthiery
Copy link
Contributor

nthiery commented Jul 11, 2016

comment:17

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.

@fchapoton
Copy link
Contributor

comment:18

bad syntax for trac link, see patchbot report

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 12, 2016

Changed commit from 9778f97 to 3c53706

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 12, 2016

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

3c53706Fixing trac link

@AndrewMathas
Copy link
Member Author

comment:20

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.

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.

@tscrim
Copy link
Collaborator

tscrim commented Jul 12, 2016

comment:21

Replying to @AndrewAtLarge:

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.

@tscrim
Copy link
Collaborator

tscrim commented Jul 12, 2016

comment:22

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2016

Changed commit from 3c53706 to c600b3c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2016

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

@tscrim
Copy link
Collaborator

tscrim commented Jul 18, 2016

Reviewer: Nicolas Thiéry, Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jul 18, 2016

comment:24

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.

@tscrim
Copy link
Collaborator

tscrim commented Jul 18, 2016

@tscrim
Copy link
Collaborator

tscrim commented Jul 18, 2016

Changed commit from c600b3c to 9cd615d

@AndrewMathas
Copy link
Member Author

comment:25

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

@vbraun
Copy link
Member

vbraun commented Jul 21, 2016

comment:26
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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2016

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

c1b57e6Fixing broken doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2016

Changed commit from 9cd615d to c1b57e6

@AndrewMathas
Copy link
Member Author

comment:28

Sorry! All fixed.

@vbraun
Copy link
Member

vbraun commented Jul 22, 2016

Changed branch from public/combinat/root_system/type_A_infinity-20973 to c1b57e6

@jdemeyer
Copy link

Changed reviewer from Nicolas Thiéry, Travis Scrimshaw to Nicolas M. Thiéry, Travis Scrimshaw

@jdemeyer
Copy link

Changed commit from c1b57e6 to none

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

6 participants