Opened 9 years ago

Closed 9 years ago

#13871 closed enhancement (fixed)

Folded Cartan types

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-5.12
Component: combinatorics Keywords: Cartan, root systems
Cc: sage-combinat, aschilling, nthiery Merged in: sage-5.12.beta5
Authors: Travis Scrimshaw Reviewers: Frédéric Chapoton, Anne Schilling, Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by tscrim)

This implements a new class CartanTypeFolded which takes a Cartan type and considers the diagram folding and stores the associated data.

This also cleans up the references in kirillov_reshetikhin.py.


Apply: trac_13871-virtual_cartan_type-ts.patch

Attachments (3)

trac_13838-virtual_kleber_alg-ts.patch (38.9 KB) - added by tscrim 9 years ago.
Fixed doctests
trac_13871_review.patch (3.5 KB) - added by chapoton 9 years ago.
trac_13871-virtual_cartan_type-ts.patch (32.1 KB) - added by tscrim 9 years ago.
Fixed doctests

Download all attachments as: .zip

Change History (38)

comment:1 Changed 9 years ago by tscrim

  • Cc sage-combinat added

comment:2 Changed 9 years ago by tscrim

  • Cc nthiery added
  • Status changed from new to needs_review

Changed 9 years ago by tscrim

Fixed doctests

comment:3 Changed 9 years ago by tscrim

  • Description modified (diff)

Uploaded wrong file...

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch

comment:4 Changed 9 years ago by tscrim

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch

comment:5 follow-up: Changed 9 years ago by chapoton

  • Description modified (diff)

found a typo here :

there exists am affine

comment:6 in reply to: ↑ 5 Changed 9 years ago by tscrim

Replying to chapoton:

found a typo here :

there exists am affine

Thanks for catching that Frederic.

comment:7 Changed 9 years ago by chapoton

  • Status changed from needs_review to needs_work

one doctest is failing (concerning latex representation)

comment:8 Changed 9 years ago by tscrim

  • Status changed from needs_work to needs_review

New version of the patch which does the changes that Nicolas and I discussed. Ready for another look.

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch

comment:9 Changed 9 years ago by tscrim

Explicitly cast the elements of scaling_factors() to python ints to simplify #13838 and #13872.

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch

comment:10 Changed 9 years ago by tscrim

Fixed typo in doctest.

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch

comment:11 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

Changed 9 years ago by chapoton

comment:12 follow-up: Changed 9 years ago by chapoton

here is a review patch, just cleaning up and putting things into pep8 shape.

Once the bot has turned green again, and if you agree with my review patch, I will give a positive review, but there is a point that bothers me :

I have never heard the words "virtual cartan type" before. Could you give a reference to an article where this terminology is used ?

comment:13 in reply to: ↑ 12 Changed 9 years ago by aschilling

Replying to chapoton:

here is a review patch, just cleaning up and putting things into pep8 shape.

Once the bot has turned green again, and if you agree with my review patch, I will give a positive review, but there is a point that bothers me :

I have never heard the words "virtual cartan type" before. Could you give a reference to an article where this terminology is used ?

Nicolas and I are going to look at the patch this week since we had some specific comments about the implementation. So please wait until we have looked at it!

Thank you,

Anne

comment:14 follow-up: Changed 9 years ago by nthiery

Hi Travis!

We finally had a brainstorm with Anne about this ticket. Sorry that they come so late in the process, but we have some suggestions on how to improve the user interface and so.

  • Documentation improvements for the main class:

Start from the definition of folding of a simply laced Dynkin diagram from :wikipedia:Dynkin_diagram (note that, unlike the current documentation suggests it's not specific to affine).

Say that the object in Sage models the Cartan type X, endowed with the folding from \hat(X).

Mention that, in the affine case, the code makes the assumption that the special node is fixed.

Mention applications to crystals, with refs.

For the scaling factors, start from a conceptual type free definition. Maybe, following [Schilling FPSAC 2006 p.4] something like: the \gamma_i are (up to overall factor?) the unique numbers so that:

\Lambda_i -> \gamma_i \sum_j \Lambda_{j\in\orbit(i)}

is a proper embedding from the weight lattice of X to that of \hat{X}?

Then give the combinatorial description in term of the Dynkin diagram and the type-free formula in affine type.

  • Suggestions for the user interface:
    sage: ct = CartanType(["G",2]); ct
    ['G',2]
    sage: vct = ct.as_folding(); vct
    ['G', 2] as folding of ['D', 4]
    sage: type(vct)
    <class '....type_folded.CartanType'>
    sage: vct.folding_of()                 # replaces virtual
    ['D', 4]
    sage: sigma = vct.folding_automorphism()       # replaces sigma
    
  • Put the implementation accordingly in .../root_system/type_folded.py
  • Does scaling_factors work for type BC relabelled?
  • Does the type free formula for the scaling factors work for type BC/dual? If not, the doc needs to be updated.

Thanks for this useful addition!

Cheers,

Anne and Nicolas

comment:15 in reply to: ↑ 14 Changed 9 years ago by aschilling

Hi Travis,

We forgot a few more things:

  • You seem to only work with the orbit of the Dynkin diagram automorphism. Hence the name
        sage: sigma = vct.folding_orbit()       # replaces sigma
    

might actually be more appropriate.

  • The output of the orbit is a dictionary. Hence it would make more sense to also take as an input a dictionary with keys the Dynkin labels. In a list it is not completely clear which label go to which (since for example in the finite case the Dynkin nodes are labeled from 1 to n rather than 0 to n). The output could actually be a family instead of a dictionary.

Thanks for your work on this!

Nicolas and Anne

comment:16 follow-up: Changed 9 years ago by tscrim

Here's the new version of the patch with your suggested changes. Thanks all for reviewing this.

Best,
Travis

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch​

comment:17 in reply to: ↑ 16 Changed 9 years ago by aschilling

Hi Travis,

Thanks for the update. Here are a couple more comments:

  • Do you really want to keep sigma as a variable or rather change it to orbit since this is what the input is? Also, when you describe what the INPUT is, please be more specific that you are inputting the orbit of a Dynkin diagram automorphism and not the automorphism itself. This is currently confusing in the documentation. Specify that this should be inputted as a dictionary.
  • Please add explanations that this is used for crystals (which results into virtual crystals) and give the appropriate references.

Thanks!

Anne

comment:18 follow-up: Changed 9 years ago by tscrim

Hey Anne and Nicolas,

Changed the input to orbit and added some more references and updated those for Kirillov-Reshetikhin crystals since I wanted to use one.

Best,
Travis

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch​

comment:19 in reply to: ↑ 18 Changed 9 years ago by aschilling

Hi Travis,

Thank you for making the changes. Could you please also add in INPUT that the orbit is a dictionary to be clear!?

Did you investigate the questions regarding relabelings that we posed?

I get doctest failures

sage -t pieri_factors.py
**********************************************************************
File "pieri_factors.py", line 352, in sage.combinat.root_system.pieri_factors.PieriFactors_affine_type.maximal_elements
Failed example:
    [w.reduced_word() for w in PF.maximal_elements()]
Expected:
    [[0, 2, 3, 2, 0], [1, 0, 2, 3, 2], [2, 3, 2, 1, 0], [3, 2, 1, 0, 2], [1, 2, 3, 2, 1], [2, 1, 0, 2, 3]]
Got:
    [[0, 2, 3, 2, 0], [1, 0, 2, 3, 2], [2, 3, 2, 1, 0], [1, 2, 3, 2, 1], [3, 2, 1, 0, 2], [2, 1, 0, 2, 3]]
**********************************************************************
File "pieri_factors.py", line 987, in sage.combinat.root_system.pieri_factors.PieriFactors_type_D_affine
Failed example:
    [u.reduced_word() for u in PF.maximal_elements()]
Expected:
    [[0, 2, 3, 5, 4, 3, 2, 0], [1, 0, 2, 3, 5, 4, 3, 2], [2, 3, 5, 4, 3, 2, 1, 0], [2, 1, 0, 2, 3, 5, 4, 3], [1, 2, 3, 5, 4, 3, 2, 1], [3, 5, 4, 3, 2, 1, 0, 2], [3, 2, 1, 0, 2, 3, 5, 4], [5, 4, 3, 2, 1, 0, 2, 3], [5, 3, 2, 1, 0, 2, 3, 5], [4, 3, 2, 1, 0, 2, 3, 4]]
Got:
    [[0, 2, 3, 5, 4, 3, 2, 0], [1, 0, 2, 3, 5, 4, 3, 2], [2, 3, 5, 4, 3, 2, 1, 0], [3, 5, 4, 3, 2, 1, 0, 2], [1, 2, 3, 5, 4, 3, 2, 1], [2, 1, 0, 2, 3, 5, 4, 3], [5, 4, 3, 2, 1, 0, 2, 3], [3, 2, 1, 0, 2, 3, 5, 4], [5, 3, 2, 1, 0, 2, 3, 5], [4, 3, 2, 1, 0, 2, 3, 4]]
**********************************************************************

though I am not sure this is caused by this patch.

After compiling the documentation the link to [FOS09] does not seem to work.

Best,

Anne

comment:20 Changed 9 years ago by aschilling

  • Reviewers set to Federic Chapoton, Anne Schilling, Nicolas Thiery

comment:21 follow-up: Changed 9 years ago by tscrim

Hey Anne,

I expanded on the doc for the INPUT: block. The Pieri factors errors occur both with and without my patch applied, so it doesn't seem to be this patch. The link to [FOS09] in both kirillov_reshetikhin.py and type_folded.py work for me. For the type BC scaling factors, I added a doctest showing it works under relabelling.

Best,
Travis

comment:22 Changed 9 years ago by tscrim

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch

comment:23 in reply to: ↑ 21 Changed 9 years ago by aschilling

Hi Travis,

Ok, looks good now. I let Frederic finish the technical review that he started!

Best,

Anne

comment:24 Changed 9 years ago by tscrim

Here's the patch with Frederic's review patch changes as well. I forgot to fold in the review patch before making Anne and Nicolas' changes.

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch

comment:25 Changed 9 years ago by chapoton

apply only trac_13871-virtual_cartan_type-ts.patch

comment:26 Changed 9 years ago by chapoton

  • Reviewers changed from Federic Chapoton, Anne Schilling, Nicolas Thiery to Frédéric Chapoton, Anne Schilling, Nicolas Thiery

comment:27 Changed 9 years ago by nthiery

  • Reviewers changed from Frédéric Chapoton, Anne Schilling, Nicolas Thiery to Frédéric Chapoton, Anne Schilling, Nicolas M. Thiéry

comment:28 Changed 9 years ago by chapoton

  • Status changed from needs_review to needs_work
  • Work issues set to doctest

there is a failing doctest that needs to be corrected

comment:29 Changed 9 years ago by tscrim

  • Status changed from needs_work to needs_review
  • Work issues doctest deleted

Here's with the fixed doctest in type_relabel.py. IDK about the one in weierstrass.py, seems completely unrelated to this patch, and it passed for me:

sage -t --long ../../schemes/toric/weierstrass.py
    [134 tests, 7.30 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 7.5 seconds
    cpu time: 6.0 seconds
    cumulative wall time: 7.3 seconds

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch

comment:30 Changed 9 years ago by chapoton

the role for arxiv is :arxiv: and not :arXiv:

otherwise, everything looks good to me

Changed 9 years ago by tscrim

Fixed doctests

comment:31 Changed 9 years ago by tscrim

Fixed.

For patchbot:

Apply: trac_13871-virtual_cartan_type-ts.patch

comment:32 Changed 9 years ago by chapoton

  • Status changed from needs_review to positive_review

ok, positive review

could you please update the description of the ticket ?

comment:33 Changed 9 years ago by tscrim

  • Description modified (diff)
  • Summary changed from Virtual Cartan types to Folded Cartan types

comment:34 Changed 9 years ago by tscrim

Thank you all for doing the review.

comment:35 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.12.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.