Opened 5 years ago

Closed 5 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 jdemeyer)

Implements infinity crystal of tableaux in types A, B, C, D, and G.

Apply: trac_14192-infinity_crystal-bs.patch

Attachments (2)

trac_14192-infinity_crystal-review-ts.patch (8.3 KB) - added by tscrim 5 years ago.
trac_14192-infinity_crystal-bs.patch (29.5 KB) - added by bsalisbury1 5 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 5 years ago by bsalisbury1

  • Status changed from new to needs_review

Changed 5 years ago by tscrim

comment:2 follow-up: Changed 5 years ago by tscrim

I've uploaded a small review patch which makes some small tweaks/fixes to the documentation.

comment:3 in reply to: ↑ 2 Changed 5 years ago by aschilling

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: Changed 5 years ago by bsalisbury1

  • 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 5 years ago by aschilling

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: Changed 5 years ago by bsalisbury1

  • Dependencies set to #14143

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

comment:7 in reply to: ↑ 6 Changed 5 years ago by aschilling

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: Changed 5 years ago by tscrim

  • Dependencies changed from #14143 to #14252

Yes...We typed in the wrong patch numbers. This does depend on #14252 and not on #14143.

For patchbot:

Apply: trac_14192-infinity_crystal-bs.patch

Last edited 5 years ago by tscrim (previous) (diff)

comment:9 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by aschilling

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 5 years ago by aschilling

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: Changed 5 years ago by aschilling

  • Reviewers set to Anne Schilling

comment:12 in reply to: ↑ 11 Changed 5 years ago by aschilling

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 5 years ago by bsalisbury1

Fixed.

comment:14 follow-up: Changed 5 years ago by aschilling

  • Dependencies #14252 deleted

comment:15 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by aschilling

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 5 years ago by aschilling

There are still trailing white spaces in the patch (in several places in infinite_crystal.py). Please remove them!

Anne

Changed 5 years ago by bsalisbury1

comment:17 follow-up: Changed 5 years ago by bsalisbury1

Removed trailing whitespace (hopefully).

comment:18 in reply to: ↑ 17 Changed 5 years ago by aschilling

Looks good!

comment:19 Changed 5 years ago by aschilling

  • Status changed from needs_review to positive_review

comment:20 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.9 to sage-5.10

comment:21 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:22 Changed 5 years ago by jdemeyer

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