Element construction for CrystalOfTableaux should be robust versus int's

[nthiery]

When constructing elements of a crystal of tableaux, the entries are converted to crystal letters if they are not yet such::

    sage: T = CrystalOfTableaux(['A',3], shape = [2,2])
    sage: t = T(list=[3,1,4,2])
    sage: t
    [[1, 2], [3, 4]]
    sage: type(t[0])
    <class 'sage.combinat.crystals.letters.ClassicalCrystalOfLetters_with_category.element_class'>
    sage: type(t[1])
    <class 'sage.combinat.crystals.letters.ClassicalCrystalOfLetters_with_category.element_class'>

However this is currently not the case if the entries are plain int's::

    sage: t = T(list=[int(3),1,4,2])
    sage: type(t[0])
    <type 'int'>
    sage: type(t[1])
    <type 'sage.rings.integer.Integer'>

Possible fix: have CrystalOfTableauxElement?.init check whether list[0] is not an instance of parent.letters.element_class instead of testing for an Integer.

[tscrim]

Another (more subtle) bug which comes from the same issue:

sage: C = KirillovReshetikhinCrystal(['A',int(4),1], 1,1)
sage: C[0].e0()
# Boom
AttributeError: 'int' object has no attribute 'epsilon'

The problem seems to be in that elements are created from C.cartan_type().n somewhere (I didn't track this down).

I've uploaded my fix for this which automatically calls the CrystalOfLetters on each element in the list (which should just type check and return the element back if it already is a letter). There is a slight performance hit (see below) for doing this rather than just checking the leading element. Also this takes care of input like:

sage: t = T(list=[T.letters(3),1,4,2])
sage: t = T(list=[CrystalOfLetters(['D',6])(3),1,4,2])

(Note CrystalOfLetters? checks (well lack thereof) are not robust enough to catch the second input as an error, but that's a separate issue.) Really the question boils down to how pathological do we think the users might be with this versus the speed hit? If people think this is a good fix, this is ready to review.

Thanks,
Travis

Performance data before patch:

sage: B = KirillovReshetikhinCrystal(['A',4,1], 3,3)

sage: %timeit L = [x for x in B]
5 loops, best of 3: 584 ms per loop
sage: %timeit L = [x for x in B]
5 loops, best of 3: 550 ms per loop

sage: %prun L = [x for x in B]
   398355 function calls in 4.147 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
    45228    0.668    0.000    1.602    0.000 crystals.py:169(index_set)
    45053    0.658    0.000    2.433    0.000 crystals.py:727(index_set)
    45228    0.443    0.000    0.680    0.000 cartan_type.py:1822(index_set)
...

after patch:

sage: B = KirillovReshetikhinCrystal(['A',4,1], 3,3)

sage: %timeit L = [x for x in B]
5 loops, best of 3: 642 ms per loop
sage: %timeit L = [x for x in B]
5 loops, best of 3: 633 ms per loop

sage: %prun L = [x for x in B]
   426299 function calls in 5.135 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
    45228    0.791    0.000    1.942    0.000 crystals.py:167(index_set)
    45053    0.728    0.000    2.874    0.000 crystals.py:726(index_set)
    45228    0.572    0.000    0.838    0.000 cartan_type.py:1822(index_set)
...

[aschilling]

Hi Travis,

Thanks for looking into this. In principle your solution is good, though I am not so happy about the performance loss. Also, it might be better to spell out the problem before your added test in the patch than (or in addition) to refer to this ticket.

Best,

Anne

[tscrim]

Here's a few other things I've tried:

With first checking an isinstance():

letters_type = type(parent.letters) # It was slightly slower to put this in the lambda function
letters_check = lambda x: x if isinstance(x, letters_type) else parent.letters(x)
TensorProductOfCrystalsElement.__init__(self, parent, map(letters_check, list))

sage: %timeit L = [x for x in B]
5 loops, best of 3: 661 ms per loop

By using the in check:

letters_check = lambda x: x if x in parent.letters else parent.letters(x)
TensorProductOfCrystalsElement.__init__(self, parent, map(letters_check, list))

sage: %timeit L = [x for x in B]
5 loops, best of 3: 852 ms per loop

Here's my baseline timing with the current patch:

sage: %timeit L = [x for x in B]
5 loops, best of 3: 610 ms per loop

[tscrim]

Added expanded documentation.

[aschilling]

I talked to Travis about this and we ran the tests. It looks good to me now.

Anne

[jdemeyer]

Small documentation problem:

/release/merger/sage-5.7.beta2/local/lib/python2.7/site-packages/sage/combinat/crystals/tensor_product.py:docstring of sage.combinat.crystals.tensor_product.CrystalOfTableauxElement:38: WARNING: Inline literal start-string without end-string.

[tscrim]

Modified docstring