Opened 4 years ago
Closed 4 years ago
#14192 closed enhancement (fixed)
Infinity crystal of tableaux
Reported by: | bsalisbury1 | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.10 |
Component: | combinatorics | Keywords: | crystal, tableaux |
Cc: | tscrim, aschilling, sage-combinat | Merged in: | sage-5.10.beta0 |
Authors: | Ben Salisbury, Travis Scrimshaw | Reviewers: | Anne Schilling |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Implements infinity crystal of tableaux in types A, B, C, D, and G.
Attachments (2)
Change History (24)
comment:1 Changed 4 years ago by
- Status changed from new to needs_review
Changed 4 years ago by
comment:2 follow-up: ↓ 3 Changed 4 years ago by
comment:3 in reply to: ↑ 2 Changed 4 years ago by
Hi Ben and Travis,
Thanks for your work on this. Here are a couple of comments:
- Could you please fold the two patches? That makes it easier to review!
- In the title "
\mathcal{B}(\infty)
Crystals of Tableaux" specify the types for which this works.
- There are trailing white spaces. Please remove them!
- Why don't you put the Element class into the parent class (by indentation)? See for example how this is done in Littelmann paths. Also, the parent class should come first!
- In string_parameters, the Examples are indented wrongly!
Please make these changes and then I'll look again!
Anne
comment:4 follow-up: ↓ 5 Changed 4 years ago by
- Description modified (diff)
Folded in review. Made appropriate changes.
For patchbot:
Apply: trac_14192-infinity_crystal-bs.patch
comment:5 in reply to: ↑ 4 Changed 4 years ago by
Thanks for making the changes! Could you also move the patch past 14143 in the sage-combinat queue and make the appropriate rebases there? 14252 is almost ready to go in as well, so we can make a dependence either way for that patch (but I think 14143 needs a little longer due to the plot patch it depends on).
Thanks!
Anne
comment:6 follow-up: ↓ 7 Changed 4 years ago by
- Dependencies set to #14143
comment:7 in reply to: ↑ 6 Changed 4 years ago by
Replying to bsalisbury1:
Made it so this does not depend on #14252 and #14252 (in the combinat queue) did not have any dependency on this patch (at least there was no fuzz that Travis and I saw).
For patchbot:
Apply trac_14192-infinity_crystal-bs.patch
Did you get this backwards? There should *not* be a dependency on 14143, but rather on 14252?
comment:8 follow-up: ↓ 9 Changed 4 years ago by
- Dependencies changed from #14143 to #14252
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 4 years ago by
Hi!
The patch needs to be exported properly. Currently the header says
# HG changeset patch # Parent b75f7b88f08e78885465bdf7149a1995337342c2
- * *
#14192: Review patch for infinity crystal of tableaux.
Anne
comment:10 in reply to: ↑ 9 Changed 4 years ago by
Hi Ben and Travis,
Here are some further comments:
- The reference [KN94] seems to be missing in the documentation. It is not clickable.
- Since you mention "... in the i-th row from the top i" you need to clarify whether you are using English or French conventions for the tableaux.
- Please say something about the shape of the tableaux you are using. I assume they can be arbitrarily large.
- Too many "the" in "...the the i-th row"
- The reference [BN10] D. Bump and M. Nakasuji. Integration on p-adic groups and crystal bases. Proc. Amer. Math. Soc. 138(5), pp. 1595–1605. is not referred to.
- The input I assume is a finite Cartan type. Please specify this in "cartan_type – A Cartan type".
- In the doctest for f you say "Return the action of e˜i on self." I guess this should be f~!
- In the doc for reduced_form, the following is not a sentence: "The reduced form of a T∈(∞) that is a tableaux with all i-boxes in the i-th row removed and if the row is empty, a ∗ is put as a placeholder." Perhaps you want to remove the "that"?
- In the documentation for string_parameters, please define the boldface i.
- In the definition of weight, there are two typos in "indendepent of the path choosen". Also, please say that the \alpha_i are simple roots.
- In InfinityCrystalOfTableauxTypeD replace "constaints" by "constraints".
So much for now!
Anne
comment:11 follow-up: ↓ 12 Changed 4 years ago by
- Reviewers set to Anne Schilling
comment:12 in reply to: ↑ 11 Changed 4 years ago by
I have another suggestion, which would be good to have as a test for the implementation. Could you program the projection to B(\lambda) (at least in a few examples) to check that the result is indeed isomorphic to a highest weight crystal?
Anne
comment:13 Changed 4 years ago by
Fixed.
comment:14 follow-up: ↓ 15 Changed 4 years ago by
- Dependencies #14252 deleted
comment:15 in reply to: ↑ 14 ; follow-up: ↓ 16 Changed 4 years ago by
I commuted 14192 past 14252 in the sage-combinat queue to get rid of the dependency. Please get the newest version of the patch from there! Could you please post the new version here? Then I think it is close to getting a positive review.
Anne
comment:16 in reply to: ↑ 15 Changed 4 years ago by
There are still trailing white spaces in the patch (in several places in infinite_crystal.py). Please remove them!
Anne
Changed 4 years ago by
comment:17 follow-up: ↓ 18 Changed 4 years ago by
Removed trailing whitespace (hopefully).
comment:18 in reply to: ↑ 17 Changed 4 years ago by
Looks good!
comment:19 Changed 4 years ago by
- Status changed from needs_review to positive_review
comment:20 Changed 4 years ago by
- Milestone changed from sage-5.9 to sage-5.10
comment:21 Changed 4 years ago by
- Description modified (diff)
comment:22 Changed 4 years ago by
- Merged in set to sage-5.10.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
I've uploaded a small review patch which makes some small tweaks/fixes to the documentation.