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: |
Description (last modified by )
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
.
Attachments (3)
Change History (38)
comment:1 Changed 9 years ago by
- Cc sage-combinat added
comment:2 Changed 9 years ago by
- Cc nthiery added
- Status changed from new to needs_review
Changed 9 years ago by
comment:3 Changed 9 years ago by
- Description modified (diff)
Uploaded wrong file...
For patchbot:
Apply: trac_13871-virtual_cartan_type-ts.patch
comment:4 Changed 9 years ago by
For patchbot:
Apply: trac_13871-virtual_cartan_type-ts.patch
comment:5 follow-up: ↓ 6 Changed 9 years ago by
- Description modified (diff)
found a typo here :
there exists am affine
comment:6 in reply to: ↑ 5 Changed 9 years ago by
comment:7 Changed 9 years ago by
- Status changed from needs_review to needs_work
one doctest is failing (concerning latex representation)
comment:8 Changed 9 years ago by
- 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
comment:10 Changed 9 years ago by
Fixed typo in doctest.
For patchbot:
Apply: trac_13871-virtual_cartan_type-ts.patch
comment:11 Changed 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
Changed 9 years ago by
comment:12 follow-up: ↓ 13 Changed 9 years ago by
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
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: ↓ 15 Changed 9 years ago by
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
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 ton
). The output could actually be a family instead of a dictionary.
Thanks for your work on this!
Nicolas and Anne
comment:16 follow-up: ↓ 17 Changed 9 years ago by
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
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: ↓ 19 Changed 9 years ago by
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
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
- Reviewers set to Federic Chapoton, Anne Schilling, Nicolas Thiery
comment:21 follow-up: ↓ 23 Changed 9 years ago by
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
For patchbot:
Apply: trac_13871-virtual_cartan_type-ts.patch
comment:23 in reply to: ↑ 21 Changed 9 years ago by
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
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
apply only trac_13871-virtual_cartan_type-ts.patch
comment:26 Changed 9 years ago by
- 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
- 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
- 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
- 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
the role for arxiv is :arxiv:
and not :arXiv:
otherwise, everything looks good to me
comment:31 Changed 9 years ago by
Fixed.
For patchbot:
Apply: trac_13871-virtual_cartan_type-ts.patch
comment:32 Changed 9 years ago by
- 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
- Description modified (diff)
- Summary changed from Virtual Cartan types to Folded Cartan types
comment:34 Changed 9 years ago by
Thank you all for doing the review.
comment:35 Changed 9 years ago by
- Merged in set to sage-5.12.beta5
- Resolution set to fixed
- Status changed from positive_review to closed
Fixed doctests