[with patch, positive review] CrystalOfTableaux call method breaks on legitimate data

[bump]

sage: C = CrystalOfTableaux(['B',2],shape=[3]) 
sage: C(rows=[[1,1,0]])

raises an exception though this is a legitimate B2 tableaux. This was analyzed by Anne Schilling and others in this thread:

 http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/cb02f961c41947e2?hl=en

[aschilling]

The patch depends on 4549 and 5308.

[aschilling]

* fixes the bug when legitimate tableau of type B is entered * allows to enter several shapes for tableaux (which will be essential for the Kirillov-Reshetikhin crystals later) * fixes a small bug in partitions when adding a box to the empty partition * fixes errors in comparison of the elements in crystal of letters

[bump]

This patch fixes three bugs.

First, there is the bug originally reported in this ticket. Second, there is the bug described at  http://groups.google.com/group/sage-combinat-devel/msg/bce932c675b8b04a?hl=en Third, it fixes an exception on Partition([]).outside_corners().

In addition there are a lot of documentation improvements.

The new class CrystalOfWords? needs a docstring. Otherwise I found no problems.

The patch does not change the effect of sage --testall.

The patch applies cleanly on sage-3.4.1.rc1.

Patch summary:

Adds index_set as a lazy_attribute method in class Crystal.
Removes class AffineCrystal which was prematurely implemented
Replaces the cmp_elements method => lt_elements in ClassicalCrystalOfLetters.
Implements __ne__, __lt__, __ge__ methods in class Letter.
Properly indents docstrings for TensorProductOfCrystals.
New class CrystalOfWords.
FullTensorProductOfCrystals inherits from CrystalOfWords.
call method of FullTensorProductOfCrystals is taken down.
CrystalOfTableaux inherits from CrystalOfWords.
Docstring revision for CrystalOfTableaux.
CrystalOfTableaux __init__ method allows multiple shapes.
CrystalOfTableaux gets new methods cartan_type and module_generator
CrystalOfTableauxElement __init__ revisions

[aschilling]

Added docstring to CrystalOfWords? as requested by Dan Bump.

[mabshoff]

Hi,

after some discussion about lazy attributed at SD 14 with Nicolas the consensus at least from the non-combinat developers was that lazy attributes should be avoided for public API data structures. If you want to access the index_set of a crystal we see no reason for the inconsistency A.data_set while everywhere else in Sage it would be A.data_set(). Caching can be done in some other ways.

Cheers,

Michael

[was]

Replying to mabshoff:

Hi, after some discussion about lazy attributed at SD 14 with Nicolas the consensus at least from the non-combinat developers was that lazy attributes should be avoided for public API data structures. If you want to access the index_set of a crystal we see no reason for the inconsistency A.data_set while everywhere else in Sage it would be A.data_set(). Caching can be done in some other ways.

+10 I very strongly agree with this. The point isn't to argue about whether lazy attributes are good or bad, but that using them is inconsistent with hundreds of thousands of lines of existing Sage code, and we're definitely not going to change all that code. For consistency, do not use them in the Sage library for "user facing" code.

[nthiery]

Peace. The lazy_attribute here is just a temporary workaround. Anne precisely did send an e-mail to the mailing list 2 days ago asking for confirmation for changing it systematically to a method in all the crystal library. This will come as a subsequent patch.

Refactoring all the crap in the combinatorics code will take a while. No need to jump on every occasion to repeat the same arguments. We have heard them, and fully taken points for them.

That being said, I still think that lazy attributes have their role to play in some well localized places in the user interface. I will run a complete discussion about this when time is ready for it. And the final code will follow whatever consensus emerges.

[bump]

Can the patch be merged (and this issue sorted out later) or does it need to be revised to avoid lazy_attributes?

I suppose this is Michael's call, so this question is addressed mainly to him.

[mabshoff]

Replying to bump:

Can the patch be merged (and this issue sorted out later) or does it need to be revised to avoid lazy_attributes? I suppose this is Michael's call, so this question is addressed mainly to him.

I am reluctant to merge any large patch that is not on my blocker list at this point in general, but I would greatly prefer if the lazy_attribute issue in this patch was fixed. The main reason is that otherwise some of the API in crystals will be different in 3.4.1 until the fix is merged and there is no clean way to handle this. I think since this is a combinat patch, i.e. no Cython, it is relatively low risk. But we are at the moment sitting around trying to get all blockers resolved and put out 3.4.1.rc2, so if the fix appears it should appear quickly.

@Nicolas: I did not see the patch by Anne, but I will look it up.

Cheers,

Michael

[aschilling]

removed reference to lazy attribute

[aschilling]

I removed the reference to lazy attribute. I hope it is now ok to be integrated. In the process I also had to touch fast_crystals.py and spins.py.

[mabshoff]

Merged crystal-5002-track.patch only in Sage 3.4.1.rc2.

I did reread the changes Anne made and they look good to me. Doctests do pass, too.

Cheers,

Michael

[mabshoff]

Merged crystal-5002-track.patch only in Sage 3.4.1.rc2.

I did reread the changes Anne made and they look good to me. Doctests do pass, too. And this time I closed the ticket, too :)

Cheers,

Michael