Opened 3 years ago

Closed 2 years ago

#22921 closed enhancement (fixed)

Implementation of shifted tableaux

Reported by: aschilling Owned by: kbparamonov
Priority: major Milestone: sage-8.2
Component: combinatorics Keywords: shifted primed tableaux, crystals
Cc: tscrim, kbparamonov, hawkes, aschilling, mantepse, andrew.mathas Merged in:
Authors: Kirill Paramonov Reviewers: Anne Schilling, Travis Scrimshaw, Martin Rubey
Report Upstream: N/A Work issues:
Branch: bf7e707 (Commits) Commit: bf7e707c6764952aeca7f2b81331cb6b6eed1738
Dependencies: Stopgaps:

Description (last modified by kbparamonov)

This ticket implements a new class for shifted primed tableaux, and an A-type crystal for those objects.

Attachments (1)

shifted_primed_tableau.py (64.4 KB) - added by mantepse 3 years ago.

Download all attachments as: .zip

Change History (170)

comment:1 Changed 3 years ago by tscrim

  • Cc tscrim added

See #20041.

comment:2 Changed 3 years ago by aschilling

  • Description modified (diff)

comment:3 Changed 3 years ago by kbparamonov

  • Cc kbparamonov added
  • Owner changed from (none) to kbparamonov

comment:4 Changed 3 years ago by hawkes

  • Cc hawkes added

comment:5 Changed 3 years ago by kbparamonov

  • Branch set to public/combinat/primed_tableaux_22921

comment:6 Changed 3 years ago by git

  • Commit set to 30437170dcb4ca118a55c9ad8f236d7f3d7227fe

Branch pushed to git repo; I updated commit sha1. New commits:

3043717File shifted_tableaux.py added in combinat.

comment:7 Changed 3 years ago by git

  • Commit changed from 30437170dcb4ca118a55c9ad8f236d7f3d7227fe to f6aa0ef473eef8401abcdb13c0876e1c73122c23

Branch pushed to git repo; I updated commit sha1. New commits:

f6aa0efNew functions and comments

comment:8 Changed 3 years ago by git

  • Commit changed from f6aa0ef473eef8401abcdb13c0876e1c73122c23 to d00e09bd336c663facf0185813d6be64df10b3a9

Branch pushed to git repo; I updated commit sha1. New commits:

d00e09bAdded proper links

comment:9 Changed 3 years ago by git

  • Commit changed from d00e09bd336c663facf0185813d6be64df10b3a9 to 13b6c8a3dc35efc14a77d4c73ecf4a8bb538c665

Branch pushed to git repo; I updated commit sha1. New commits:

13b6c8aadded crystal structure

comment:10 follow-up: Changed 3 years ago by aschilling

  • Cc aschilling added

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by aschilling

Looking at the code, I do not think it is necessary to implement a new class for StrictPartitions?. You can access those using the already existing Partitions code:

sage: Partitions(7, max_slope=-1).list()
[[7], [6, 1], [5, 2], [4, 3], [4, 2, 1]]

Please also make sure that you stick to the Sage coding conventions described here ​http://doc.sagemath.org/html/en/developer/coding_basics.html

comment:12 in reply to: ↑ 11 ; follow-up: Changed 3 years ago by aschilling

I still get many doctest failures (here are two, but there are many more):

sage -t tableau_shifted_primed.py
**********************************************************************
File "tableau_shifted_primed.py", line 43, in sage.combinat.tableau_shifted_primed.ShiftedPrimedTableau
Failed example:
    T([[1,"2'","3'",3],[2,"3'"]])[1]
Exception raised:
    Traceback (most recent call last):
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 509, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 872, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.tableau_shifted_primed.ShiftedPrimedTableau[1]>", line 1, in <module>
        T([[Integer(1),"2'","3'",Integer(3)],[Integer(2),"3'"]])[Integer(1)]
      File "sage/structure/parent.pyx", line 941, in sage.structure.parent.Parent.__call__ (/Applications/sage/src/build/cythonized/sage/structure/parent.c:9839)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 110, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (/Applications/sage/src/build/cythonized/sage/structure/coerce_maps.c:4895)
        raise
      File "sage/structure/coerce_maps.pyx", line 105, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (/Applications/sage/src/build/cythonized/sage/structure/coerce_maps.c:4762)
        return C._element_constructor(x)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/combinat/tableau_shifted_primed.py", line 904, in _element_constructor_
        raise ValueError("{} is not an element of {}".format(t, self))
    ValueError: [[1, "2'", "3'", 3], [2, "3'"]] is not an element of Shifted Primed tableaux of shape [4, 2]
**********************************************************************
File "tableau_shifted_primed.py", line 45, in sage.combinat.tableau_shifted_primed.ShiftedPrimedTableau
Failed example:
    t = ShiftedPrimedTableau([[1,"2p",2.5,3],[0,2,2.5]])
Exception raised:
    Traceback (most recent call last):
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 509, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 872, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.tableau_shifted_primed.ShiftedPrimedTableau[2]>", line 1, in <module>
        t = ShiftedPrimedTableau([[Integer(1),"2p",RealNumber('2.5'),Integer(3)],[Integer(0),Integer(2),RealNumber('2.5')]])
      File "sage/misc/lazy_import.pyx", line 389, in sage.misc.lazy_import.LazyImport.__call__ (/Applications/sage/src/build/cythonized/sage/misc/lazy_import.c:4016)
        return self._get_object()(*args, **kwds)
      File "sage/misc/classcall_metaclass.pyx", line 330, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (/Applications/sage/src/build/cythonized/sage/misc/classcall_metaclass.c:1415)
        return cls.classcall(cls, *args, **kwds)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/combinat/tableau_shifted_primed.py", line 86, in __classcall_private__
        return ShiftedPrimedTableaux(shape = shape, weight = weight)(T)
      File "sage/structure/parent.pyx", line 941, in sage.structure.parent.Parent.__call__ (/Applications/sage/src/build/cythonized/sage/structure/parent.c:9839)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 110, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (/Applications/sage/src/build/cythonized/sage/structure/coerce_maps.c:4895)
        raise
      File "sage/structure/coerce_maps.pyx", line 105, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (/Applications/sage/src/build/cythonized/sage/structure/coerce_maps.c:4762)
        return C._element_constructor(x)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/combinat/tableau_shifted_primed.py", line 1004, in _element_constructor_
        raise ValueError("{} is not an element of {}".format(t, self))
    ValueError: [[1, '2p', 2.50000000000000, 3], [0, 2, 2.50000000000000]] is not an element of Shifted Primed Tableaux of weight (1, 4, 1) and shape [4, 2]
Last edited 3 years ago by aschilling (previous) (diff)

comment:13 in reply to: ↑ 12 Changed 3 years ago by aschilling

Here are some more trivial comments:

  • you need a blank line before and after EXAMPLES::
  • Returnes -> Return (several times)
  • you need a blank line after the one-line description of a given method
  • you need a 4-space indent of the code for each EXAMPLES:: This indent is sometimes missing.
  • EXAMPE: -> EXAMPLES::
  • There is still some documentation missing for some of the methods

comment:14 Changed 3 years ago by git

  • Commit changed from 13b6c8a3dc35efc14a77d4c73ecf4a8bb538c665 to 5dee1d6915e838b7c87e34d77e5dd417670157e1

Branch pushed to git repo; I updated commit sha1. New commits:

5dee1d6Fixed functions

comment:15 Changed 3 years ago by git

  • Commit changed from 5dee1d6915e838b7c87e34d77e5dd417670157e1 to d582dcd0cd10cd32847bcf6df6fe1a7478b0c7be

Branch pushed to git repo; I updated commit sha1. New commits:

d582dcdAdded documentation, cleaned up the code

comment:16 Changed 3 years ago by git

  • Commit changed from d582dcd0cd10cd32847bcf6df6fe1a7478b0c7be to 5bc82dcf2b4659def502f089910b3ea09a234b0e

Branch pushed to git repo; I updated commit sha1. New commits:

5bc82dcFixed doctests

comment:17 Changed 3 years ago by mantepse

In #23319 I have just implemented a growth diagram version of the insertion algorithm...

comment:18 Changed 3 years ago by kbparamonov

  • Reviewers set to aschilling, tscrim
  • Status changed from new to needs_review

comment:19 Changed 3 years ago by git

  • Commit changed from 5bc82dcf2b4659def502f089910b3ea09a234b0e to 84093c4bc19d2aa7e353dfa2b2436a5bf56b96b8

Branch pushed to git repo; I updated commit sha1. New commits:

84093c4fix minor formatting issues

comment:20 follow-up: Changed 3 years ago by mantepse

It would be nice to get this in, so we can have the following from #23896

sage: G = GrowthDiagramShiftedShapes([5, 10, 8, 7, 4, 1, 2, 9, 3, 6])
sage: G.P_symbol().pp()
 1   2   3   6   9
     4   5   7
         8   10

sage: G.Q_symbol().pp()
1   2   4'  5'  6'
    3   7'  8
        9   10
Last edited 3 years ago by mantepse (previous) (diff)

comment:21 Changed 3 years ago by git

  • Commit changed from 84093c4bc19d2aa7e353dfa2b2436a5bf56b96b8 to 724d7b64830f3c5ba6143bdbadae40060ca1f91d

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

8853225fix a bug and add more (failing) tests
995d6d6fix final fehler, produce P_symbol as StrongTableau
1ddf882add rank function
500e234docfixes
7027638doc fixes (in particular use the global reference file) and simplify defaults
a1db015make r a class attribute and implement test for Domino as an example
1ca9e7aMerge branch 'develop' into t/23319/growth_diagram_improvements
6b2a647add documentation and doctests
5928543Merge branch 'u/mantepse/growth_diagram_improvements' of git://trac.sagemath.org/sage into t/22921/public/combinat/primed_tableaux_22921
724d7b6more trivial fixes

comment:22 Changed 3 years ago by mantepse

Oh dear, help needed. All the commits after

​84093c4 fix minor formatting issues

should go away!

comment:23 Changed 3 years ago by git

  • Commit changed from 724d7b64830f3c5ba6143bdbadae40060ca1f91d to 84093c4bc19d2aa7e353dfa2b2436a5bf56b96b8

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

comment:24 Changed 3 years ago by git

  • Commit changed from 84093c4bc19d2aa7e353dfa2b2436a5bf56b96b8 to ff08b90e26c4f28070189ea194aed518908f3103

Branch pushed to git repo; I updated commit sha1. New commits:

ff08b90more trivial fixes

comment:25 Changed 3 years ago by mantepse

I just realised that we should have a skew version, too.

comment:26 Changed 3 years ago by git

  • Commit changed from ff08b90e26c4f28070189ea194aed518908f3103 to 3c84bbd502038a6973b7f7d3d67c1c6b76732d0e

Branch pushed to git repo; I updated commit sha1. New commits:

3c84bbdNew Sage version

comment:27 in reply to: ↑ 20 ; follow-up: Changed 3 years ago by kbparamonov

Thanks for the minor fixes! Let me know if you have further feedback. I'd rather do a semistandard version of Haiman insertion here, after the base class is implemented.

Replying to mantepse:

It would be nice to get this in, so we can have the following from #23896

sage: G = GrowthDiagramShiftedShapes([5, 10, 8, 7, 4, 1, 2, 9, 3, 6])
sage: G.P_symbol().pp()
 1   2   3   6   9
     4   5   7
         8   10

sage: G.Q_symbol().pp()
1   2   4'  5'  6'
    3   7'  8
        9   10

comment:28 Changed 3 years ago by kbparamonov

  • Cc mantepse added

comment:29 in reply to: ↑ 27 ; follow-up: Changed 3 years ago by mantepse

Replying to kbparamonov:

Thanks for the minor fixes! Let me know if you have further feedback.

Well, the most important thing for me would be support for the skew version, and support for _ascii_art_. (In principle I think that there should be generic support for all sorts of fillings of shapes, but that's certainly not in the scope of this ticket.)

Important question: why do you use ClonableArray instead of ClonableList, as Tableau does?

I'd rather do a semistandard version of Haiman insertion here, after the base class is implemented.

As you like - maybe you could also do the skew version? You can use the growth diagram version for cross-checking the standard case - the ticket is already closed, so it will be in 8.1.beta7. The syntax has changed slightly, it is now

GrowthDiagram.rules.ShiftedShapes()([5,10,8,7,4,1,2,9,3,6])

I am too lazy to implement the semistandard version of the local rules, and I think they should be done generically anyway, via standardization and destandardization of the growth diagram.

Last edited 3 years ago by mantepse (previous) (diff)

comment:30 in reply to: ↑ 29 Changed 3 years ago by kbparamonov

Replying to mantepse:

Well, the most important thing for me would be support for the skew version, and support for _ascii_art_. (In principle I think that there should be generic support for all sorts of fillings of shapes, but that's certainly not in the scope of this ticket.)

Got it

Important question: why do you use ClonableArray instead of ClonableList, as Tableau does?

Good point, ClonableList is more consistent.

Last edited 3 years ago by kbparamonov (previous) (diff)

comment:31 Changed 3 years ago by git

  • Commit changed from 3c84bbd502038a6973b7f7d3d67c1c6b76732d0e to ef251e17f4b118b02d5d0acf5e8b489309e50b01

Branch pushed to git repo; I updated commit sha1. New commits:

ef251e1improved style and doctests

comment:32 follow-up: Changed 3 years ago by tscrim

Tableau probably should be using ClonableArray instead of ClonableList because it (should have) a fixed size. From the doc of structure/list_clone.pyx:

- :class:`ClonableArray` for arrays (lists of fixed length) of objects;
- :class:`ClonableList` for (resizable) lists of objects;

comment:33 in reply to: ↑ 32 Changed 3 years ago by mantepse

Replying to tscrim:

Tableau probably should be using ClonableArray instead of ClonableList because it (should have) a fixed size.

OK, I misunderstood: apparently, size is the number of rows. So, a tableau really should be a ClonableArray of lists, right? Or should it be a ClonableArray of ClonableArrays?

comment:34 Changed 3 years ago by tscrim

ClonableArray of tuples is best so the rows cannot be modified via T[0] (a la #15862).

comment:35 Changed 3 years ago by git

  • Commit changed from ef251e17f4b118b02d5d0acf5e8b489309e50b01 to 20aefa8cc222c75a176a819ff1f1a8640f4768fa

Branch pushed to git repo; I updated commit sha1. New commits:

20aefa8Added Ascii and unicode

comment:36 Changed 3 years ago by mantepse

Great! One further comment (apart from the skew thing): it might be nice to have, similar to what's provided by SkewTableau, an argument chain that takes a sequence of shapes and creates a shifted (skew) tableau. This is already implemented in #23896 (methods P_symbol and Q_symbol) for the standard case, but should be straightforward also for the (skew) semistandard case. You can simply take the code from there, I would then replace those with a call to ShiftedPrimedTableau(chain=Q_chain).

I admit that I'm not completely sure about how to do it right: in the shifted case, a chain is a sequence of strict partitions, alternating with a colour:

  • colour 1 (black) - add a box on the diagonal
  • colour 2 (blue) - add an ordinary box
  • colour 3 (red) - add a primed box

Of course, black is redundant, so maybe you just want to have two colours...

comment:37 Changed 3 years ago by aschilling

Hi Kirill,

Not all methods seems to have documentation, see

sage -coverage tableau_shifted_primed.py 
------------------------------------------------------------------------
SCORE tableau_shifted_primed.py: 86.8% (46 of 53)

Missing documentation:
     * line 1630: def __classcall_private__(cls, *args, **kwargs)
     * line 1660: def __init__(self, shape=[4, 2], n=2)
     * line 1669: def _repr_(self)
     * line 1678: def add_strip(sub_tab, full_tab, length)
     * line 1736: def preprocessing(T)

Missing doctests:
     * line 1243: def __init__(self, shape, max_elt)
     * line 1510: def __init__(self, weight, shape)
------------------------------------------------------------------------

comment:38 Changed 3 years ago by aschilling

How do you actually make the crystal of shifted primed tableaux? There is no documentation given for this. Also, you want to include the crystal in the catalogue of crystals, so that it will be accessible as crystals.shifted_primed_tableaux(shape). Currently, this does not seem to be implemented.

comment:39 Changed 3 years ago by mantepse

  • Status changed from needs_review to needs_work

comment:40 Changed 3 years ago by git

  • Commit changed from 20aefa8cc222c75a176a819ff1f1a8640f4768fa to bf9567d6df5adeddc1218e9c5a513e0f1681281e

Branch pushed to git repo; I updated commit sha1. New commits:

bf9567dAdded crystal documentation, put in the crystal catalog

comment:41 follow-up: Changed 3 years ago by mantepse

Is there a good reason to have shape=(4,2) and n=2 as defaults?

comment:42 in reply to: ↑ 41 Changed 3 years ago by aschilling

Replying to mantepse:

Is there a good reason to have shape=(4,2) and n=2 as defaults?

No, those should be removed! Examples should be given with these values, but there should not be defaults!

comment:43 Changed 3 years ago by git

  • Commit changed from bf9567d6df5adeddc1218e9c5a513e0f1681281e to 9fa940624649de1c9bdcefc3f6e279be037cca29

Branch pushed to git repo; I updated commit sha1. New commits:

9fa9406No default value for the shape of the crystal

comment:44 follow-ups: Changed 3 years ago by mantepse

  • There are leftover default values in line 1740.
  • I think list_highest_weight is redundant. What I'm not sure about: shouldn't some of the methods, in particular list_decreasing_weight, be methods of the crystal class?
    sage: %timeit crystals.ShiftedPrimedTableaux(shape=[4,3,2,1]).highest_weight_vectors()
    1 loop, best of 3: 24.9 s per loop
    sage: %timeit ShiftedPrimedTableaux(shape=[4,3,2,1]).list_highest_weight()
    1 loop, best of 3: 24.3 s per loop
    
  • The documentation in line 1648 is not clear to me, isn't it just constructed applying the lowering operators (you could link to those using :meth:...) to the highest weights?
  • I think that to_matrix should be a private method - suppose that the internal representation changes, then it possibly doesn't make much sense anymore.
  • I'm a bit unsure about reading_word_with_positions, possibly that should be private, too.
  • any chance for the skew version?

comment:45 in reply to: ↑ 44 Changed 3 years ago by kbparamonov

Replying to mantepse:

  • There are leftover default values in line 1740.

Those are for factory class ShiftedPrimedTableauxCrystal, which is not supposed to be called directly, so default values are only for internal use. I'll change it to raise error if arguments are not passed.

  • I think list_highest_weight is redundant. What I'm not sure about: shouldn't some of the methods, in particular list_decreasing_weight, be methods of the crystal class?
    sage: %timeit crystals.ShiftedPrimedTableaux(shape=[4,3,2,1]).highest_weight_vectors()
    1 loop, best of 3: 24.9 s per loop
    sage: %timeit ShiftedPrimedTableaux(shape=[4,3,2,1]).list_highest_weight()
    1 loop, best of 3: 24.3 s per loop
    

list_highest_weight is interesting in terms of the paper we've been working on, the length of that list would be related to a certain Schur decomposition.

  • The documentation in line 1648 is not clear to me, isn't it just constructed applying the lowering operators (you could link to those using :meth:...) to the highest weights?

I'll include better explanation.

  • I think that to_matrix should be a private method - suppose that the internal representation changes, then it possibly doesn't make much sense anymore.

Agree.

  • I'm a bit unsure about reading_word_with_positions, possibly that should be private, too.

Agree.

  • any chance for the skew version?

I'll be modifying the functions for the skew version. Expect it next week.

Last edited 3 years ago by kbparamonov (previous) (diff)

comment:46 Changed 3 years ago by mantepse

Please clarify: isn't list_highest_weight the same as crystals.ShiftedPrimedTableaux(shape=[4,3,2,1]).highest_weight_vectors()?

comment:47 in reply to: ↑ 44 Changed 3 years ago by aschilling

Replying to mantepse:

  • any chance for the skew version?

The skew version should be in a new ticket. Let us first get the crystal into Sage. Then you can add code to deal with the skew cases.

comment:48 Changed 3 years ago by mantepse

OK, sounds reasonable!

comment:49 Changed 3 years ago by git

  • Commit changed from 9fa940624649de1c9bdcefc3f6e279be037cca29 to 2cc0e9b0183adb4f74661c847325f2c309e7a79c

Branch pushed to git repo; I updated commit sha1. New commits:

2cc0e9bFixed minor issues

comment:50 Changed 3 years ago by aschilling

Hi Kirill,

You changed the name of the crystal class as we discussed, but you did not change the tests. Now there are a lot of failing tests!

comment:51 Changed 3 years ago by aschilling

There are still missing doctests:

Missing doctests:
     * line 1172: def __init__(self, shape, max_elt)
     * line 1439: def __init__(self, weight, shape)
     * line 1710: def add_strip(sub_tab, full_tab, length)

comment:52 Changed 3 years ago by git

  • Commit changed from 2cc0e9b0183adb4f74661c847325f2c309e7a79c to a9a52ca23dec0bc018c8d7d264d2fa263b3e17e1

Branch pushed to git repo; I updated commit sha1. New commits:

a9a52caFixed doctests

comment:53 follow-up: Changed 3 years ago by git

  • Commit changed from a9a52ca23dec0bc018c8d7d264d2fa263b3e17e1 to f6e95408910a26d45f6370edfbbb300a9ee3bd2b

Branch pushed to git repo; I updated commit sha1. New commits:

f6e9540Fixed documentation

comment:54 in reply to: ↑ 53 ; follow-up: Changed 3 years ago by aschilling

Hi Kirill,

I still get a lot of doctest failures when running the tests on 'tableau_shifted_primed.py'

sage -t tableau_shifted_primed.py  # 171 doctests failed

Can you please fix them?

Thanks,

Anne

comment:55 in reply to: ↑ 54 Changed 3 years ago by kbparamonov

Hi Anne,

I don't know why that happened, it passed all the tests for me.

Replying to aschilling:

Hi Kirill,

I still get a lot of doctest failures when running the tests on 'tableau_shifted_primed.py'

sage -t tableau_shifted_primed.py  # 171 doctests failed

Can you please fix them?

Thanks,

Anne

comment:56 follow-up: Changed 3 years ago by aschilling

I get the following

sage: B = crystals.ShiftedPrimedTableaux([3,2],2)
sage: TestSuite(B).run(verbose=True)
running ._test_an_element() . . . pass
running ._test_cardinality() . . . pass
running ._test_category() . . . pass
running ._test_elements() . . .
  Running the test suite of self.an_element()
  running ._test_category() . . . pass
  running ._test_eq() . . . pass
  running ._test_new() . . . pass
  running ._test_not_implemented_methods() . . . pass
  running ._test_pickling() . . . pass
  pass
running ._test_elements_eq_reflexive() . . . pass
running ._test_elements_eq_symmetric() . . . pass
running ._test_elements_eq_transitive() . . . pass
running ._test_elements_neq() . . . pass
running ._test_enumerated_set_contains() . . . pass
running ._test_enumerated_set_iter_cardinality() . . . pass
running ._test_enumerated_set_iter_list() . . . pass
running ._test_eq() . . . pass
running ._test_fast_iter() . . . pass
running ._test_new() . . . pass
running ._test_not_implemented_methods() . . . pass
running ._test_pickling() . . . pass
running ._test_some_elements() . . . pass
running ._test_stembridge_local_axioms() . . . fail
Traceback (most recent call last):
  File "/Applications/sage/local/lib/python2.7/site-packages/sage/misc/sage_unittest.py", line 294, in run
    test_method(tester = tester)
  File "/Applications/sage/local/lib/python2.7/site-packages/sage/categories/regular_crystals.py", line 313, in _test_stembridge_local_axioms
    goodness = x._test_stembridge_local_axioms(index_set, verbose)
  File "sage/structure/element.pyx", line 484, in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4368)
    return self.getattr_from_category(name)
  File "sage/structure/element.pyx", line 497, in sage.structure.element.Element.getattr_from_category (build/cythonized/sage/structure/element.c:4477)
    return getattr_from_other_class(self, cls, name)
  File "sage/cpython/getattr.pyx", line 249, in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:1830)
    raise dummy_attribute_error
AttributeError: 'ShiftedPrimedTableaux_all_with_category.element_class' object has no attribute '_test_stembridge_local_axioms'
------------------------------------------------------------
The following tests failed: _test_stembridge_local_axioms

comment:57 in reply to: ↑ 56 ; follow-up: Changed 3 years ago by kbparamonov

It looks like the tableau class doesn't inherit method _test_stembridge_local_axioms. Do you know why this happens?

Replying to aschilling:

I get the following

sage: B = crystals.ShiftedPrimedTableaux([3,2],2)
sage: TestSuite(B).run(verbose=True)
running ._test_an_element() . . . pass
running ._test_cardinality() . . . pass
running ._test_category() . . . pass
running ._test_elements() . . .
  Running the test suite of self.an_element()
  running ._test_category() . . . pass
  running ._test_eq() . . . pass
  running ._test_new() . . . pass
  running ._test_not_implemented_methods() . . . pass
  running ._test_pickling() . . . pass
  pass
running ._test_elements_eq_reflexive() . . . pass
running ._test_elements_eq_symmetric() . . . pass
running ._test_elements_eq_transitive() . . . pass
running ._test_elements_neq() . . . pass
running ._test_enumerated_set_contains() . . . pass
running ._test_enumerated_set_iter_cardinality() . . . pass
running ._test_enumerated_set_iter_list() . . . pass
running ._test_eq() . . . pass
running ._test_fast_iter() . . . pass
running ._test_new() . . . pass
running ._test_not_implemented_methods() . . . pass
running ._test_pickling() . . . pass
running ._test_some_elements() . . . pass
running ._test_stembridge_local_axioms() . . . fail
Traceback (most recent call last):
  File "/Applications/sage/local/lib/python2.7/site-packages/sage/misc/sage_unittest.py", line 294, in run
    test_method(tester = tester)
  File "/Applications/sage/local/lib/python2.7/site-packages/sage/categories/regular_crystals.py", line 313, in _test_stembridge_local_axioms
    goodness = x._test_stembridge_local_axioms(index_set, verbose)
  File "sage/structure/element.pyx", line 484, in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4368)
    return self.getattr_from_category(name)
  File "sage/structure/element.pyx", line 497, in sage.structure.element.Element.getattr_from_category (build/cythonized/sage/structure/element.c:4477)
    return getattr_from_other_class(self, cls, name)
  File "sage/cpython/getattr.pyx", line 249, in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:1830)
    raise dummy_attribute_error
AttributeError: 'ShiftedPrimedTableaux_all_with_category.element_class' object has no attribute '_test_stembridge_local_axioms'
------------------------------------------------------------
The following tests failed: _test_stembridge_local_axioms

comment:58 in reply to: ↑ 57 Changed 3 years ago by tscrim

Replying to kbparamonov:

It looks like the tableau class doesn't inherit method _test_stembridge_local_axioms. Do you know why this happens?

Here is the problem:

sage: SPTC = crystals.ShiftedPrimedTableaux([4,2], 2)
sage: SPTC.an_element()
[(1.0, 1.0, 1.0, 1.0), (2.0, 2.0)]
sage: type(_)
<class 'sage.combinat.tableau_shifted_primed.ShiftedPrimedTableaux_all_with_category.element_class'>
sage: type(SPTC)
<class 'sage.combinat.tableau_shifted_primed.SPTCrystal_with_category'>
sage: SPTC.element_class
<class 'sage.combinat.tableau_shifted_primed.SPTCrystal_with_category.element_class'>

The elements of SPTC are not subclasses of SPTC.element_class. Hence, they belong to the wrong category.

sage: SPTC.an_element().category()
Category of elements of Shifted Primed Tableaux

I also do not see why you have ShiftedPrimedTableauxCrystal and the subclass SPTCrystal. It seems like the SPT crystals really should be the same as the SPT with a fixed shape and max element, but I've only quickly looked at it.

I am pushing a few simple tweaks. We can work on this more tomorrow.

comment:59 Changed 3 years ago by git

  • Commit changed from f6e95408910a26d45f6370edfbbb300a9ee3bd2b to 841c08d8edaf4ddcebedccf9081e05355db06f5d

Branch pushed to git repo; I updated commit sha1. New commits:

8942346Merge branch 'public/combinat/primed_tableaux_22921' of git://trac.sagemath.org/sage into public/combinat/primed_tableaux_22921
841c08dSome simple doc formatting things.

comment:60 Changed 3 years ago by git

  • Commit changed from 841c08d8edaf4ddcebedccf9081e05355db06f5d to 8d6bbfef2cbf3865c72e630ee1eb0ddcb0bed639

Branch pushed to git repo; I updated commit sha1. New commits:

3fe8dccInitial skew implementation
3b6fb54Skew
f4b7371More skew
5d556dfSkewed
86ca57bSkewed version
8d6bbfeSkewed version

comment:61 Changed 3 years ago by git

  • Commit changed from 8d6bbfef2cbf3865c72e630ee1eb0ddcb0bed639 to 0efbae46f953d923d9c18e30e47615b3ad4b2665

Branch pushed to git repo; I updated commit sha1. New commits:

0efbae4Some cleanup and simplification of the class structure.

comment:62 follow-up: Changed 3 years ago by andrew.mathas

Can you please add a reference to the literature to the documentation for shifted primed tableaux? I'm actually really curious to see a reference, independent of the code, because in my work "shifted shaded tableaux" come up and I wonder if they are the same objects. From a cursory look at the documentation they seem quite close.

comment:63 Changed 3 years ago by andrew.mathas

  • Reviewers changed from aschilling, tscrim to aschilling, tscrim, andrew.mathas

comment:64 Changed 3 years ago by andrew.mathas

  • Cc andrew.mathas added
  • Reviewers changed from aschilling, tscrim, andrew.mathas to aschilling, tscrim

comment:65 Changed 3 years ago by tscrim

Graham, Kirill, or Anne can probably answer better than I can, but a good starting point would be their paper:

.. [HPS2017] Graham Hawkes, Kirill Paramonov, and Anne Schilling.
             *Crystal analysis of type* `C` *Stanley symmetric functions*.
             Preprint, :arxiv:`1704.00889`.

comment:66 in reply to: ↑ 62 ; follow-up: Changed 3 years ago by kbparamonov

I believe those tableaux were first introduced by Haiman here: http://www.sciencedirect.com/science/article/pii/0097316589900150 and recently were developed by Serrano here: https://arxiv.org/abs/0811.2057

Replying to andrew.mathas:

Can you please add a reference to the literature to the documentation for shifted primed tableaux? I'm actually really curious to see a reference, independent of the code, because in my work "shifted shaded tableaux" come up and I wonder if they are the same objects. From a cursory look at the documentation they seem quite close.

comment:67 in reply to: ↑ 66 ; follow-up: Changed 3 years ago by aschilling

Replying to kbparamonov:

I believe those tableaux were first introduced by Haiman here: http://www.sciencedirect.com/science/article/pii/0097316589900150 and recently were developed by Serrano here: https://arxiv.org/abs/0811.2057

Kirill, could you please add those references to the code? Also, did you fix the Stembridge axiom issues?

comment:68 in reply to: ↑ 67 Changed 3 years ago by kbparamonov

Sounds good. Travis fixed the Stembridge axioms issues last week.

Replying to aschilling:

Replying to kbparamonov:

I believe those tableaux were first introduced by Haiman here: http://www.sciencedirect.com/science/article/pii/0097316589900150 and recently were developed by Serrano here: https://arxiv.org/abs/0811.2057

Kirill, could you please add those references to the code? Also, did you fix the Stembridge axiom issues?

comment:69 Changed 3 years ago by git

  • Commit changed from 0efbae46f953d923d9c18e30e47615b3ad4b2665 to b8b49c4a04529794cc4402e5cc2a4160d1131b3d

Branch pushed to git repo; I updated commit sha1. New commits:

086b424Merge branch 'develop' into public/combinat/primed_tableaux_22921
4932e35Merge branch 'develop' into public/combinat/primed_tableaux_22921
b8b49c4fixed some typos in 22921

comment:70 follow-up: Changed 3 years ago by aschilling

  • Reviewers changed from aschilling, tscrim to Anne Schilling, Travis Scrimshaw

Hi Kirill,

Ok, let us try to get this ticket into Sage! I have tested the code extensively and the crystals seem to be correct. I rebased the code on the latest development version and also fixed some small typos.

There are still some small issues with the coverage:

sage -coverage tableau_shifted_primed.py 
------------------------------------------------------------------------
SCORE tableau_shifted_primed.py: 95.9% (47 of 49)

Missing documentation:
     * line 1305: def __classcall_private__(cls, shape, max_elt=None, skew=None)

Missing doctests:
     * line 1416: def module_generators(self)

Possibly wrong (function name doesn't occur in doctests):
     * line 1755: def add_strip(sub_tab, full_tab, length)
------------------------------------------------------------------------

Could you please fix those?

Also, the ticket description mentions that this code is based on #20041. Can this be removed now? It also mentions various insertion algorithms, but I think they are not implemented in this ticket. Could you please update the description?

Thank you!

Anne

comment:71 Changed 3 years ago by mantepse

That would be great!

THe insertion algorithms are available via #23896, which could be reviewed as soon as this is done! (Hint :-)

comment:72 Changed 3 years ago by git

  • Commit changed from b8b49c4a04529794cc4402e5cc2a4160d1131b3d to 64cb74d9fb91952dfa504193376ca8c8be9e2718

Branch pushed to git repo; I updated commit sha1. New commits:

64cb74dFixed numpy issues and missing docs.

comment:73 Changed 3 years ago by kbparamonov

  • Description modified (diff)
  • Keywords primed crystals added; insertion algorithms removed
  • Status changed from needs_work to needs_review

comment:74 in reply to: ↑ 70 Changed 3 years ago by kbparamonov

Hi Anne,

I've removed the use of numpy in the code, could you check if everything still works? Other issues have been fixed.

Replying to aschilling:

Hi Kirill,

Ok, let us try to get this ticket into Sage! I have tested the code extensively and the crystals seem to be correct. I rebased the code on the latest development version and also fixed some small typos.

There are still some small issues with the coverage:

sage -coverage tableau_shifted_primed.py 
------------------------------------------------------------------------
SCORE tableau_shifted_primed.py: 95.9% (47 of 49)

Missing documentation:
     * line 1305: def __classcall_private__(cls, shape, max_elt=None, skew=None)

Missing doctests:
     * line 1416: def module_generators(self)

Possibly wrong (function name doesn't occur in doctests):
     * line 1755: def add_strip(sub_tab, full_tab, length)
------------------------------------------------------------------------

Could you please fix those?

Also, the ticket description mentions that this code is based on #20041. Can this be removed now? It also mentions various insertion algorithms, but I think they are not implemented in this ticket. Could you please update the description?

Thank you!

Anne

comment:75 Changed 3 years ago by aschilling

Hi Kirill,

Thanks for the fixes. There seems to be some problem with the documentation:

[dochtml] [combinat ] /Applications/sage/local/lib/python2.7/site-packages/sage/combinat/tableau_shifted_primed.py:docstring of sage.combinat.tableau_shifted_primed.ShiftedPrimedTableaux_shape:9: WARNING: citation not found: HPS17

Anne

comment:76 follow-up: Changed 3 years ago by git

  • Commit changed from 64cb74d9fb91952dfa504193376ca8c8be9e2718 to b1d7d2e55e04a298a3a611c041dfa6bfa74adf2c

Branch pushed to git repo; I updated commit sha1. New commits:

b1d7d2eFixed reference to the paper

comment:77 in reply to: ↑ 76 Changed 3 years ago by kbparamonov

Alright, should be fixed.

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

b1d7d2eFixed reference to the paper

comment:78 follow-ups: Changed 3 years ago by mantepse

I think that shape should return a skew partition, when necessary, and a partition otherwise. Currently it returns lists. For example:

        if self._skew is None:
            return Partition([len(_) for _ in self])
        return SkewPartition(([len(self[i])+self._skew[i]
                               for i in range(len(self._skew))] +
                              [len(self[i])
                               for i in range(len(self._skew), len(self))],
                              self._skew))

(doctests would have to be adapted)

comment:79 Changed 3 years ago by mantepse

I am not sure whether implementing __call__ for shifted tableaux is a good idea. In any case, it is not quite clear to me what it does:

sage: s = ShiftedPrimedTableau([["2p",3,4],[2]],skew=[2,1])
sage: s.pp()
 .  .  2' 3  4 
    .  2 
sage: s(0,0)
3.0
sage: s(0,1)
4.0
sage: s(0,2)
1.5
sage: s(0,3)
3.0
sage: s(0,4)
4.0
sage: s(0,5)
IndexError: invalid cell
sage: s(1,0)
2.0
sage: s(1,1)
2.0
sage: s(1,2)
IndexError: invalid cell
sage: s(2,0)
IndexError: invalid cell

comment:80 in reply to: ↑ 78 ; follow-up: Changed 3 years ago by aschilling

Replying to mantepse:

I think that shape should return a skew partition, when necessary, and a partition otherwise. Currently it returns lists. For example:

        if self._skew is None:
            return Partition([len(_) for _ in self])
        return SkewPartition(([len(self[i])+self._skew[i]
                               for i in range(len(self._skew))] +
                              [len(self[i])
                               for i in range(len(self._skew), len(self))],
                              self._skew))

(doctests would have to be adapted)

Hi Kirill,

Thank you for your changes. I agree with Martin that the shape should be in Partitions or SkewPartitions. In fact, the same is true for the weight of a crystal element (cf. with the weight of a tableaux crystal element):

sage: B = crystals.ShiftedPrimedTableaux([4,3,1], 5)
sage: b = B.an_element()
sage: type(b.weight())
<type 'tuple'>
sage: B = crystals.Tableaux(['A',2],shape=[2,1])
sage: b = B.an_element()
sage: type(b.weight())
<class 'sage.combinat.root_system.type_A.AmbientSpace_with_category.element_class'>

Anne

comment:81 Changed 3 years ago by git

  • Commit changed from b1d7d2e55e04a298a3a611c041dfa6bfa74adf2c to 8aaf077b7c3fd4183a9c2b41f6e46ef83cd4a93d

Branch pushed to git repo; I updated commit sha1. New commits:

c02b807Merge branch 'develop' into public/combinat/primed_tableaux_22921
8aaf077Fix some documentation

comment:82 Changed 3 years ago by aschilling

Hi Kirill,

I merged in the latest development branch and also fixed some documentation. Please use this version when you fix the shape and weight to be of the correct type.

Thank you,

Anne

comment:83 Changed 3 years ago by git

  • Commit changed from 8aaf077b7c3fd4183a9c2b41f6e46ef83cd4a93d to df0f1192d57d634290cad6612257bbed2e6a84ba

Branch pushed to git repo; I updated commit sha1. New commits:

df0f119Removed call, changed shape type

comment:84 in reply to: ↑ 78 Changed 3 years ago by kbparamonov

Good point, thank you!

Replying to mantepse:

I think that shape should return a skew partition, when necessary, and a partition otherwise. Currently it returns lists. For example:

        if self._skew is None:
            return Partition([len(_) for _ in self])
        return SkewPartition(([len(self[i])+self._skew[i]
                               for i in range(len(self._skew))] +
                              [len(self[i])
                               for i in range(len(self._skew), len(self))],
                              self._skew))

(doctests would have to be adapted)

comment:85 in reply to: ↑ 80 ; follow-up: Changed 3 years ago by kbparamonov

That makes sense. How do I change type of the weight of crystal elements?

Replying to aschilling:

Replying to mantepse:

I think that shape should return a skew partition, when necessary, and a partition otherwise. Currently it returns lists. For example:

        if self._skew is None:
            return Partition([len(_) for _ in self])
        return SkewPartition(([len(self[i])+self._skew[i]
                               for i in range(len(self._skew))] +
                              [len(self[i])
                               for i in range(len(self._skew), len(self))],
                              self._skew))

(doctests would have to be adapted)

Hi Kirill,

Thank you for your changes. I agree with Martin that the shape should be in Partitions or SkewPartitions. In fact, the same is true for the weight of a crystal element (cf. with the weight of a tableaux crystal element):

sage: B = crystals.ShiftedPrimedTableaux([4,3,1], 5)
sage: b = B.an_element()
sage: type(b.weight())
<type 'tuple'>
sage: B = crystals.Tableaux(['A',2],shape=[2,1])
sage: b = B.an_element()
sage: type(b.weight())
<class 'sage.combinat.root_system.type_A.AmbientSpace_with_category.element_class'>

Anne

comment:86 in reply to: ↑ 85 ; follow-up: Changed 3 years ago by aschilling

Replying to kbparamonov:

That makes sense. How do I change type of the weight of crystal elements?

You can do

sage: B = crystals.ShiftedPrimedTableaux([4,3,1], 5)
sage: b = B.an_element()
sage: b
[(1.0, 1.0, 1.0, 1.0), (2.0, 2.0, 2.0), (3.0,)]
sage: B.weight_lattice_realization()(b.weight())
(4, 3, 1, 0, 0)

comment:87 in reply to: ↑ 86 Changed 3 years ago by kbparamonov

I'm still not sure how to implement this.

Replying to aschilling:

Replying to kbparamonov:

That makes sense. How do I change type of the weight of crystal elements?

You can do

sage: B = crystals.ShiftedPrimedTableaux([4,3,1], 5)
sage: b = B.an_element()
sage: b
[(1.0, 1.0, 1.0, 1.0), (2.0, 2.0, 2.0), (3.0,)]
sage: B.weight_lattice_realization()(b.weight())
(4, 3, 1, 0, 0)

comment:88 follow-up: Changed 3 years ago by mantepse

        weight = tuple([flat.count(i+1) for i in range(max_ind)])
        if self._skew is None:
            P = ShiftedPrimedTableaux(self.shape(), max_element=max_ind)
            return P.weight_lattice_realization()(weight)
        return weight

(needs more doctesting)

comment:89 Changed 3 years ago by git

  • Commit changed from df0f1192d57d634290cad6612257bbed2e6a84ba to 8850224e15f67795770e75a9762a9c129991ffbf

Branch pushed to git repo; I updated commit sha1. New commits:

8850224Fixed weight type for crystals, minor skew tableaux fixes.

comment:90 in reply to: ↑ 88 Changed 3 years ago by kbparamonov

Thanks for help!

Replying to mantepse:

        weight = tuple([flat.count(i+1) for i in range(max_ind)])
        if self._skew is None:
            P = ShiftedPrimedTableaux(self.shape(), max_element=max_ind)
            return P.weight_lattice_realization()(weight)
        return weight

(needs more doctesting)

comment:91 follow-up: Changed 3 years ago by mantepse

Are you sure that you want to test only the following?

if isinstance(self.parent(), ShiftedPrimedTableaux_shape):

I don't know - should weight only return an element of the AmbientSpace when the tableau is initialised with a specific shape?

comment:92 follow-up: Changed 3 years ago by mantepse

Finally, I am not yet convinced that list_highest_weight and possibly also list_decreasing_weight should be there, see comment:44 above.

comment:93 in reply to: ↑ 91 Changed 3 years ago by kbparamonov

I need

    return self.parent().weight_lattice_realization()(weight)

for the fixed-shape tableaux to get the crystal right, which doesn't work for other parents. I didn't add that for other parents because we need to change the weight type only for crystal elements. It's not hard to fix this though.

Replying to mantepse:

Are you sure that you want to test only the following?

if isinstance(self.parent(), ShiftedPrimedTableaux_shape):

I don't know - should weight only return an element of the AmbientSpace when the tableau is initialised with a specific shape?

comment:94 Changed 3 years ago by git

  • Commit changed from 8850224e15f67795770e75a9762a9c129991ffbf to 1da4b48716cc80492cce88ddfa5739d8fe0ff34c

Branch pushed to git repo; I updated commit sha1. New commits:

1da4b48Removed list_highest_weight

comment:95 in reply to: ↑ 92 ; follow-up: Changed 3 years ago by kbparamonov

I think list_decreasing_weight is useful to get module generators for the crystal.

Replying to mantepse:

Finally, I am not yet convinced that list_highest_weight and possibly also list_decreasing_weight should be there, see comment:44 above.

comment:96 in reply to: ↑ 95 Changed 3 years ago by tscrim

Replying to kbparamonov:

I think list_decreasing_weight is useful to get module generators for the crystal.

Replying to mantepse:

Finally, I am not yet convinced that list_highest_weight and possibly also list_decreasing_weight should be there, see comment:44 above.

You could just move that code into module_generators as list_decreasing_weight is not used elsewhere.

comment:97 Changed 3 years ago by git

  • Commit changed from 1da4b48716cc80492cce88ddfa5739d8fe0ff34c to 1a03e0b7947ad9ed06a1e21b81cd5660880972ff

Branch pushed to git repo; I updated commit sha1. New commits:

1a03e0bRemoved list_decreasing_weight

comment:98 follow-ups: Changed 3 years ago by mantepse

I'm afraid I have some more requests, please excuse me.

  • for all other varieties of tableaux (eg. CompositionTableaux, SemiStandardTableaux) the maximal entry is
        - ``size`` -- the size of the composition tableaux
        - ``shape`` -- the shape of the composition tableaux
        - ``max_entry`` -- the maximum entry for the composition tableaux
    

so I propose to rename both the parameter and the method from max_element to max_entry. Also, the alias max_elt should be removed.

  • the alias _is_a should be removed
  • this is more a question: should the method is_highest_weight really be included? It would be the only instance of a separately defined is_highest_weight, all other crystals use the definition in categories/crystals.py.
  • currently, modules concerning tableaux follow the naming scheme adjective_tableau.py:
    composition_tableau.py
    k_tableau.py
    lr_tableau.py
    ribbon_shaped_tableau.py
    ribbon_tableau.py
    skew_tableau.py
    

so I propose to rename the module to shifted_primed_tableau.py.

  • The docstring of weight should be
            Return the weight of ``self``.
    
  • The docstring of pp should be
            Pretty print ``self``.
    
  • the doctest of add_strip (should this have a leading underscore?) should have an # indirect doctest flag:
        TESTS::
    
            sage: list(ShiftedPrimedTableaux([3,1],(2,2)))                # indirect doctest
            [[(1.0, 1.0, 2.0), (2.0,)], [(1.0, 1.0, 1.5), (2.0,)]]
    
    

After this, and if Anne and Travis agree, I'd say the ticked is done! Many many thanks! I will then update #23896 so we have the insertion algorithms, too.

comment:99 in reply to: ↑ 98 Changed 3 years ago by aschilling

I agree with Martin's comments! is_highest_weight should only be implemented if the explicit implementation is faster than the default one.

comment:100 Changed 3 years ago by aschilling

  • Reviewers changed from Anne Schilling, Travis Scrimshaw to Anne Schilling, Travis Scrimshaw, Martin Rubey

comment:101 Changed 3 years ago by git

  • Commit changed from 1a03e0b7947ad9ed06a1e21b81cd5660880972ff to 9e2a43d054b4d9fab9f5ee33e51433223f7c0d99

Branch pushed to git repo; I updated commit sha1. New commits:

eab9d4fMinor fixes
9e2a43dMinor fixes

comment:102 in reply to: ↑ 98 ; follow-up: Changed 3 years ago by kbparamonov

Thank you for your suggestions! I agree with everything, except for is_highest_weight comment: my implementation checks whether the reading word is Yamanouchi, which is a bit fast than checking each operator e(i) directly.

Replying to mantepse:

I'm afraid I have some more requests, please excuse me.

  • for all other varieties of tableaux (eg. CompositionTableaux, SemiStandardTableaux) the maximal entry is
        - ``size`` -- the size of the composition tableaux
        - ``shape`` -- the shape of the composition tableaux
        - ``max_entry`` -- the maximum entry for the composition tableaux
    

so I propose to rename both the parameter and the method from max_element to max_entry. Also, the alias max_elt should be removed.

  • the alias _is_a should be removed
  • this is more a question: should the method is_highest_weight really be included? It would be the only instance of a separately defined is_highest_weight, all other crystals use the definition in categories/crystals.py.
  • currently, modules concerning tableaux follow the naming scheme adjective_tableau.py:
    composition_tableau.py
    k_tableau.py
    lr_tableau.py
    ribbon_shaped_tableau.py
    ribbon_tableau.py
    skew_tableau.py
    

so I propose to rename the module to shifted_primed_tableau.py.

  • The docstring of weight should be
            Return the weight of ``self``.
    
  • The docstring of pp should be
            Pretty print ``self``.
    
  • the doctest of add_strip (should this have a leading underscore?) should have an # indirect doctest flag:
        TESTS::
    
            sage: list(ShiftedPrimedTableaux([3,1],(2,2)))                # indirect doctest
            [[(1.0, 1.0, 2.0), (2.0,)], [(1.0, 1.0, 1.5), (2.0,)]]
    
    

After this, and if Anne and Travis agree, I'd say the ticked is done! Many many thanks! I will then update #23896 so we have the insertion algorithms, too.

Last edited 3 years ago by kbparamonov (previous) (diff)

comment:103 in reply to: ↑ 102 ; follow-up: Changed 3 years ago by tscrim

Replying to kbparamonov:

Thank you for your suggestions! I agree with everything, except for is_highest_weight comment: my implementation checks whether the reading word is Yamanouchi, which is a bit fast than checking each operator e(i) directly.

Please post your timings. Also, to be consistent with the crystal implementation of is_highest_weight, it must support an optional index_set argument.

comment:104 Changed 3 years ago by mantepse

I actually do get better performance. Kirill's version:

sage: %timeit [a for a in ShiftedPrimedTableaux(shape=[5,3,2], max_entry=4) if a.is_highest_weight()]
1 loop, best of 3: 5.12 s per loop
sage: L = [a for a in ShiftedPrimedTableaux(shape=[5,3,2], max_entry=4)]
sage: len(L)
960
sage: %timeit [a for a in L if a.is_highest_weight()]
10 loops, best of 3: 187 ms per loop

Generic version:

sage: %timeit [a for a in ShiftedPrimedTableaux(shape=[5,3,2], max_entry=4) if a.is_highest_weight()]
1 loop, best of 3: 5.66 s per loop
sage: L = [a for a in ShiftedPrimedTableaux(shape=[5,3,2], max_entry=4)]
sage: len(L)
960
sage: %timeit [a for a in L if a.is_highest_weight()]
1 loop, best of 3: 825 ms per loop

comment:105 in reply to: ↑ 103 ; follow-up: Changed 3 years ago by aschilling

Kirill, I think you forgot to rename the file in the documentation links:

[dochtml] OSError: [combinat ] WARNING: /Applications/sage/src/doc/en/reference/combinat/sage/combinat/tableau_shifted_primed.rst:11: (WARNING/2) autodoc: failed to import module u'sage.combinat.tableau_shifted_primed'; the following exception was raised:

comment:106 in reply to: ↑ 105 ; follow-up: Changed 3 years ago by kbparamonov

What file is responsible for generating the documentation?

Replying to aschilling:

Kirill, I think you forgot to rename the file in the documentation links:

[dochtml] OSError: [combinat ] WARNING: /Applications/sage/src/doc/en/reference/combinat/sage/combinat/tableau_shifted_primed.rst:11: (WARNING/2) autodoc: failed to import module u'sage.combinat.tableau_shifted_primed'; the following exception was raised:

comment:107 in reply to: ↑ 106 Changed 3 years ago by tscrim

Replying to kbparamonov:

What file is responsible for generating the documentation?

src/doc/en/reference/combinat/module_list.rst

Addendum: you also need to change src/sage/combinat/catalog_partitions.py.

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

comment:108 follow-up: Changed 3 years ago by tscrim

I know its somewhat far into this, but I don't like using floating points for the half integers. They are inexact and have a bad representation (e.g., when you raise an error on an invalid object). I think you should have a class that represents the letters in your alphabet. You will have the logic more localized and can better handle input and gives you better control over your letter's behavior. It's more work, but I think it will result in cleaner and easier to maintain code. Internally, you can do half integers (which I would also represent with elements in QQ).

Here are some additional comments:

  • Revert the changes in combinat/partition.py as they are not substantive and could create unnecessary merge conflicts.
  • Your ShiftedPrimedTableau.__init__ should have a check argument that passes it off to ClonableArray.__init__.
  • It is good practice to not use _ as an iterating variable, e.g., t = [tuple(_) for _ in t] (normally it is done this way when it is a dummy variable).
  • You need to also implement a __ne__:
    sage: t = ShiftedPrimedTableau([[1,"2p"]])
    sage: t != ShiftedPrimedTableaux([2])([1,1.5])
    True
    
  • I think the checking that the shape is a partition should not be done within check but in _element_constructor_.
  • For the _repr_, you should link to the global options of Tableaux and mimic that.
  • The fact that you use max_entry, which is rounded up, leads to an extra space in the repr for a max entry of 9'.
  • Is the _to_matrix() suppose to only handle non-skew shapes? In particular, I am worried about the m = self.shape()[0].
  • You should put latex format docstring, such as `i` in weight.
  • Your indentation of numbered lists is 1 too many (it should be at the same level as the rest of the documentation).
  • In f and e, self should never be None.
  • Instead of self.parent()(T), you should use type(self)(self.parent(), T).
  • (As previously mentioned) is_highest_weight needs to take an index_set argument.
  • Classes do not have an OUTPUT. Also, I prefer to have INPUT after the descriptive text of a class (and methods) doc.
  • Anytime you have latex in a doc, you should make the string a raw string by doing r""".
  • None of the Shifted Primed Tableaux classes can be iterated over. is not true.
  • Capitalization of Shifted Primed Tableaux should follow usual English capitalization (it is not a proper noun) in the documentation (not for the _repr_).
  • ShiftedPrimedTableaux is not a factory class, so the __classcall_private__ is wrong. For that doc, I would just say "Normalize and process input to return the correct parent and ensure a unique representation."
  • Be explicit about your arguments in __classcall_private__ instead of having to do extra processing.
  • Do not be afraid to break the 80 char/line in code. For example, I would not do a line break here:
                          raise ValueError(
                              'invalid argument for weight or shape')
    
  • The ShiftedPrimedTableaux.__contains__ looks effectively like the default one from Parent, so I think it could be removed.
  • Similarly for ShiftedPrimedTableaux_all._element_constructor_, which looses a more specific error message by the extra handling.
  • I do not understand what you mean by nonincreasing weight.
  • Actually, now that I've gotten through more of the code, I think you should implement your check by checking T in self.parent() and implementing all the actual checks in the __contains__.
  • For the crystals, you can use the iterator from the category.
  • Infinite sets are not a reason you cannot iterate.
  • For the __contains__ and _element_constructor_, use super calls.
  • For the __iter__, it is not standard practice to return a generator object.
  • You do not need to repeatedly do Element = ShiftedPrimedTableau.
  • You should have a subclass of ShiftedPrimedTableau specifically for the case when it is a crystal.
  • You have a blank yield in ShiftedPrimedTableaux_weight_shape.__iter__, and yield is not a function.
  • In here Partition(sorted(list(self._weight), key=int, reverse=True))):, I don't understand why you need the key=int or why you need or should sort self._weight.

comment:109 in reply to: ↑ 108 Changed 3 years ago by kbparamonov

Alright, I'll be working on your comments.

Replying to tscrim:

I know its somewhat far into this, but I don't like using floating points for the half integers. They are inexact and have a bad representation (e.g., when you raise an error on an invalid object). I think you should have a class that represents the letters in your alphabet. You will have the logic more localized and can better handle input and gives you better control over your letter's behavior. It's more work, but I think it will result in cleaner and easier to maintain code. Internally, you can do half integers (which I would also represent with elements in QQ).

Here are some additional comments:

  • Revert the changes in combinat/partition.py as they are not substantive and could create unnecessary merge conflicts.
  • Your ShiftedPrimedTableau.__init__ should have a check argument that passes it off to ClonableArray.__init__.
  • It is good practice to not use _ as an iterating variable, e.g., t = [tuple(_) for _ in t] (normally it is done this way when it is a dummy variable).
  • You need to also implement a __ne__:
    sage: t = ShiftedPrimedTableau([[1,"2p"]])
    sage: t != ShiftedPrimedTableaux([2])([1,1.5])
    True
    
  • I think the checking that the shape is a partition should not be done within check but in _element_constructor_.
  • For the _repr_, you should link to the global options of Tableaux and mimic that.
  • The fact that you use max_entry, which is rounded up, leads to an extra space in the repr for a max entry of 9'.
  • Is the _to_matrix() suppose to only handle non-skew shapes? In particular, I am worried about the m = self.shape()[0].
  • You should put latex format docstring, such as `i` in weight.
  • Your indentation of numbered lists is 1 too many (it should be at the same level as the rest of the documentation).
  • In f and e, self should never be None.
  • Instead of self.parent()(T), you should use type(self)(self.parent(), T).
  • (As previously mentioned) is_highest_weight needs to take an index_set argument.
  • Classes do not have an OUTPUT. Also, I prefer to have INPUT after the descriptive text of a class (and methods) doc.
  • Anytime you have latex in a doc, you should make the string a raw string by doing r""".
  • None of the Shifted Primed Tableaux classes can be iterated over. is not true.
  • Capitalization of Shifted Primed Tableaux should follow usual English capitalization (it is not a proper noun) in the documentation (not for the _repr_).
  • ShiftedPrimedTableaux is not a factory class, so the __classcall_private__ is wrong. For that doc, I would just say "Normalize and process input to return the correct parent and ensure a unique representation."
  • Be explicit about your arguments in __classcall_private__ instead of having to do extra processing.
  • Do not be afraid to break the 80 char/line in code. For example, I would not do a line break here:
                          raise ValueError(
                              'invalid argument for weight or shape')
    
  • The ShiftedPrimedTableaux.__contains__ looks effectively like the default one from Parent, so I think it could be removed.
  • Similarly for ShiftedPrimedTableaux_all._element_constructor_, which looses a more specific error message by the extra handling.
  • I do not understand what you mean by nonincreasing weight.
  • Actually, now that I've gotten through more of the code, I think you should implement your check by checking T in self.parent() and implementing all the actual checks in the __contains__.
  • For the crystals, you can use the iterator from the category.
  • Infinite sets are not a reason you cannot iterate.
  • For the __contains__ and _element_constructor_, use super calls.
  • For the __iter__, it is not standard practice to return a generator object.
  • You do not need to repeatedly do Element = ShiftedPrimedTableau.
  • You should have a subclass of ShiftedPrimedTableau specifically for the case when it is a crystal.
  • You have a blank yield in ShiftedPrimedTableaux_weight_shape.__iter__, and yield is not a function.
  • In here Partition(sorted(list(self._weight), key=int, reverse=True))):, I don't understand why you need the key=int or why you need or should sort self._weight.

comment:110 Changed 3 years ago by git

  • Commit changed from 9e2a43d054b4d9fab9f5ee33e51433223f7c0d99 to de04f86ca1fcbe96a14d6b3fc802a9f86d29d197

Branch pushed to git repo; I updated commit sha1. New commits:

560deb4Corrected the name to properly generate docs
28561f1Revert changes to partition.py
f041576Various fixes
de04f86Added iter

comment:111 follow-up: Changed 3 years ago by git

  • Commit changed from de04f86ca1fcbe96a14d6b3fc802a9f86d29d197 to ea347f74dd96d9955622a930f9658560e0ead65d

Branch pushed to git repo; I updated commit sha1. New commits:

ea347f7Added class of entries

comment:112 in reply to: ↑ 111 Changed 3 years ago by kbparamonov

Alright, I've addressed Travis' comments. The code is ready for another review.

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

ea347f7Added class of entries
Last edited 3 years ago by kbparamonov (previous) (diff)

comment:113 Changed 3 years ago by aschilling

The documentation does not compile:

[dochtml] OSError: [combinat ] /Applications/sage/local/lib/python2.7/site-packages/sage/combinat/shifted_primed_tableau.py:docstring of sage.combinat.shifted_primed_tableau.CrystalElementShiftedPrimedTableau.reading_word:10: WARNING: Unexpected indentation.
[dochtml] 
make[2]: *** [doc-html] Error 1

comment:114 Changed 3 years ago by git

  • Commit changed from ea347f74dd96d9955622a930f9658560e0ead65d to 464c2a913f0c6cc1f2451f5324fd69a4b129a857

Branch pushed to git repo; I updated commit sha1. New commits:

464c2a9Fixed indentation

comment:115 Changed 3 years ago by mantepse

The weight of a skew shifted tableau currently fails:

sage: s = ShiftedPrimedTableau([(1, 3.5), (2.5,), (6,)], skew=[2,1])
sage: s.weight()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-194-c0897c3f66a5> in <module>()
----> 1 s.weight()

/home/martin/sage-develop/src/sage/combinat/shifted_primed_tableau.py in weight(self)
    697            (1, 4, 1)
    698 
--> 699            sage: s = ShiftedPrimedTableau([(1, 3.5), (2.5,), (6,)], skew=[2,1])
    700            sage: s.weight()
    701         """

AttributeError: 'NoneType' object has no attribute 'unprime'

A fix would be

    def weight(self):
        r"""
        Return the weight of ``self``.

        The weight of a shifted primed tableau is defined to be the vector
        with `i`-th component equal to the number of entries i and i' in the
        tableau.

        EXAMPLES::

           sage: t = ShiftedPrimedTableau([[1,'2p',2,2],[2,'3p']])
           sage: t.weight()
           (1, 4, 1)

           sage: s = ShiftedPrimedTableau([(1, 3.5), (2.5,), (6,)], skew=[2,1])
           sage: s.weight()
           (1, 0, 1, 1, 0, 1)
        """
        flat = [entry.unprime() for row in self for entry in row if entry is not None]
        if flat == []:
            max_ind = 0
        else:
            max_ind = max(flat)
        weight = tuple([flat.count(i+1) for i in range(max_ind)])
        return weight

It might be good to test a skew tableau in all methods.

comment:116 follow-up: Changed 3 years ago by mantepse

I am not completely sure, but it might be that there is also a speed regression:

martin@Martin-Laptop:~/sage-develop$ sage -t src/sage/combinat/shifted_primed_tableau.py 
too many failed tests, not using stored timings
Running doctests with ID 2018-01-21-14-48-10-0647444a.
Git branch: HEAD
Using --optional=atlas,database_gap,dot2tex,fricas,gap_packages,mpir,ore_algebra,python2,sage
Doctesting 1 file.
sage -t src/sage/combinat/shifted_primed_tableau.py
    [218 tests, 48.25 s]

I think it was much faster before, but I did not take notes :-(

comment:117 Changed 3 years ago by git

  • Commit changed from 464c2a913f0c6cc1f2451f5324fd69a4b129a857 to 6428f276ba874e86931357ebde1d1f813764d8ac

Branch pushed to git repo; I updated commit sha1. New commits:

6428f27Fixed weight for skew tableaux

comment:118 in reply to: ↑ 116 Changed 3 years ago by kbparamonov

Thanks for comments, Martin! I had a speedup on my computer actually.

sage -t shifted_primed_tableau.py
    [199 tests, 11.27 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 11.4 seconds
    cpu time: 11.0 seconds
    cumulative wall time: 11.3 seconds

Replying to mantepse:

I am not completely sure, but it might be that there is also a speed regression:

martin@Martin-Laptop:~/sage-develop$ sage -t src/sage/combinat/shifted_primed_tableau.py 
too many failed tests, not using stored timings
Running doctests with ID 2018-01-21-14-48-10-0647444a.
Git branch: HEAD
Using --optional=atlas,database_gap,dot2tex,fricas,gap_packages,mpir,ore_algebra,python2,sage
Doctesting 1 file.
sage -t src/sage/combinat/shifted_primed_tableau.py
    [218 tests, 48.25 s]

I think it was much faster before, but I did not take notes :-(

comment:119 Changed 3 years ago by git

  • Commit changed from 6428f276ba874e86931357ebde1d1f813764d8ac to 7f73b362bcc7ec335dc92c690def9abc2837a3e9

Branch pushed to git repo; I updated commit sha1. New commits:

7f73b36Minor documentation fix

comment:120 Changed 3 years ago by aschilling

For me the tests on shifted_primed_tableau also only take about 9 seconds. There are still a couple of methods without proper tests:

sage -coverage shifted_primed_tableau.py 
------------------------------------------------------------------------
SCORE shifted_primed_tableau.py: 93.2% (55 of 59)

Missing doctests:
     * line 140: def _preprocess_(T, skew=None)
     * line 1128: def __init__(self, skew=None)
     * line 1134: def _element_constructor_(self, T)
     * line 1153: def _contains_tableau_(self, T)

Possibly wrong (function name doesn't occur in doctests):
     * line 241: def _repr_list(self)
------------------------------------------------------------------------

After that I am happy to set a positive review unless other issues arise!

comment:121 Changed 3 years ago by mantepse

I checked, on my office computer tests also take only about 8 seconds. On my laptop, there is precisely one test which takes more than one second:

Trying (line 1502):    TestSuite(ShiftedPrimedTableaux([4,2,1], max_entry=4)).run()
Expecting nothing
ok [44.84 s]

comment:122 follow-up: Changed 3 years ago by mantepse

The following change makes quite a difference. After the change, most of the time is spent in QQ(entry).

diff --git a/src/sage/combinat/shifted_primed_tableau.py b/src/sage/combinat/shifted_primed_tableau.py
index 266e556..8057497 100644
--- a/src/sage/combinat/shifted_primed_tableau.py
+++ b/src/sage/combinat/shifted_primed_tableau.py
@@ -884,15 +884,14 @@ class ShiftedPrimedTableauEntry(Rational):
             sage: ShiftedPrimedTableau([[1,1.5]])[0][1]
             2'
         """
+        half = Integer(1)/Integer(2)
         if isinstance(entry, str):
             if (entry[-1] == "'" or entry[-1] == "p") and entry[:-1].isdigit() is True:
                 # Check if an element has "'" or "p" at the end
-                entry = QQ(entry[:-1]) - .5
+                entry = QQ(entry[:-1]) - half
         try:
-            if (QQ(entry)+.5 in ZZ) or (QQ(entry) in ZZ):
-                # Check if an element is a half-integer
-                entry = QQ(entry)
-            else:
+            entry = QQ(entry)
+            if (entry not in ZZ) and ((entry + half) not in ZZ):
                 raise ValueError("all numbers must be half-integers")
         except (TypeError, ValueError):
             raise ValueError("primed elements have wrong format")

comment:123 Changed 3 years ago by git

  • Commit changed from 7f73b362bcc7ec335dc92c690def9abc2837a3e9 to e312dfe11f392ccd5c67675a7ee31481aa8e888d

Branch pushed to git repo; I updated commit sha1. New commits:

e312dfeMore doctests

comment:124 Changed 3 years ago by git

  • Commit changed from e312dfe11f392ccd5c67675a7ee31481aa8e888d to 89a9fce92e10534dce169f8bf7b60d0e2b1f89ec

Branch pushed to git repo; I updated commit sha1. New commits:

89a9fceSpeedup init for entry class

comment:125 in reply to: ↑ 122 Changed 3 years ago by kbparamonov

Try it now, it should work faster.

Replying to mantepse:

The following change makes quite a difference. After the change, most of the time is spent in QQ(entry).

diff --git a/src/sage/combinat/shifted_primed_tableau.py b/src/sage/combinat/shifted_primed_tableau.py
index 266e556..8057497 100644
--- a/src/sage/combinat/shifted_primed_tableau.py
+++ b/src/sage/combinat/shifted_primed_tableau.py
@@ -884,15 +884,14 @@ class ShiftedPrimedTableauEntry(Rational):
             sage: ShiftedPrimedTableau([[1,1.5]])[0][1]
             2'
         """
+        half = Integer(1)/Integer(2)
         if isinstance(entry, str):
             if (entry[-1] == "'" or entry[-1] == "p") and entry[:-1].isdigit() is True:
                 # Check if an element has "'" or "p" at the end
-                entry = QQ(entry[:-1]) - .5
+                entry = QQ(entry[:-1]) - half
         try:
-            if (QQ(entry)+.5 in ZZ) or (QQ(entry) in ZZ):
-                # Check if an element is a half-integer
-                entry = QQ(entry)
-            else:
+            entry = QQ(entry)
+            if (entry not in ZZ) and ((entry + half) not in ZZ):
                 raise ValueError("all numbers must be half-integers")
         except (TypeError, ValueError):
             raise ValueError("primed elements have wrong format")

comment:126 Changed 3 years ago by git

  • Commit changed from 89a9fce92e10534dce169f8bf7b60d0e2b1f89ec to f21e876ad790c8d8ae7c1ed9d7dbbf16be5951a2

Branch pushed to git repo; I updated commit sha1. New commits:

f21e876More speedup init for entry class

comment:127 follow-up: Changed 3 years ago by mantepse

Indeed, it is faster now. However, I now realise that I do not understand the logic of ShiftedPrimedTableauEntry.__init__:

  • removing the call to float in ShiftedPrimedTableauEntry.__add__ makes __init__ break - why?
  • the check for half-integer could also be done using entry.denominator() in (1, 2).

comment:128 Changed 3 years ago by git

  • Commit changed from f21e876ad790c8d8ae7c1ed9d7dbbf16be5951a2 to 582099c36863459524e292afab9d33c271b941c9

Branch pushed to git repo; I updated commit sha1. New commits:

582099cFixed addition on primed entries, speedup of crystal operators

comment:129 Changed 3 years ago by git

  • Commit changed from 582099c36863459524e292afab9d33c271b941c9 to ed56f4b50be919d5be7c89a8a0238560861f3f01

Branch pushed to git repo; I updated commit sha1. New commits:

ed56f4bMinor fix for checking half-integer

comment:130 Changed 3 years ago by git

  • Commit changed from ed56f4b50be919d5be7c89a8a0238560861f3f01 to fe58704b1a69db9856d397cec4a4a726fbda74c1

Branch pushed to git repo; I updated commit sha1. New commits:

fe58704Minor fix

comment:131 in reply to: ↑ 127 Changed 3 years ago by kbparamonov

Alright, I've reworked the add function, it was too janky.

Replying to mantepse:

Indeed, it is faster now. However, I now realise that I do not understand the logic of ShiftedPrimedTableauEntry.__init__:

  • removing the call to float in ShiftedPrimedTableauEntry.__add__ makes __init__ break - why?
  • the check for half-integer could also be done using entry.denominator() in (1, 2).

comment:132 Changed 3 years ago by mantepse

It is much faster now, and easier to understand!

Very tiny thing: I would suggest to rename

    def unprime(self):

to

    def unprimed(self):

because it doesn't modify self (of course).

Travis, could you please do the final check?

comment:133 Changed 3 years ago by tscrim

Sorry, I haven't had time the past few days and most likely will not for the next few (but I will try). However, on a quick look, I do not like the tableau entries inheriting from Rational because they do not behave like rationals: they do not have a parent object, they do not have "actual" arithmetic, and they are not rational numbers in the usual sense. In a way, you override the methods precisely why you are inheriting from Rational. I think you would be better wrapping a Rational, but I would like to Cythonize it (which I can do once I have a few minutes).

comment:134 follow-up: Changed 3 years ago by mantepse

Travis, isn't that an implementation detail, mostly private? A lot of time already went into this ticket, I would think it might be better to go in for now...

Anyway I also thought a wrapper would be more logical. To be precise, it should be a poset, but that's probably overkill.

EDIT: corrected version.

class PrimedEntry(SageObject):
    """
    The class of entries in shifted primed tableaux.
    """
    def __init__(self, entry=None, double=None):
        """
        Normalize the entry.

        TEST::

            sage: ShiftedPrimedTableau([[1,"2p"]])[0][1]
            2'
            sage: ShiftedPrimedTableau([[1,"2'"]])[0][1]
            2'
            sage: ShiftedPrimedTableau([[1,1.5]])[0][1]
            2'
        """
        # store primed numbers as odd, unprimed numbers as even integers
        if isinstance(entry, self.__class__):
            self._entry = entry._entry
            return

        if double is not None:
            self._entry = Integer(double)
            return

        if isinstance(entry, str):
            if (entry[-1] == "'" or entry[-1] == "p") and entry[:-1].isdigit() is True:
                # Check if an element has "'" or "p" at the end
                self._entry = 2*Integer(entry[:-1]) - 1
                return

        try:
            entry = Rational(entry)
        except (TypeError, ValueError):
            raise ValueError("primed elements have wrong format")
        if entry.denominator() not in (1, 2):
            # Check if an element is a half-integer
            raise ValueError("all numbers must be half-integers")
        self._entry = Integer(2*entry)

    def __hash__(self):
        return self._entry

    def __repr__(self):
        """
        Represent ``self`` as primed or unprimed integer.

        TEST::

            sage: ShiftedPrimedTableau([[1,"2p"]])[0][1]
            2'
        """
        if is_even(self._entry):
            return str(self._entry//2)
        else:
            return str((self._entry+1)//2) + "'"

    def integer(self):
        return self._entry//2

    def __eq__(self, other):
        return self._entry == PrimedEntry(other)._entry

    def __ne__(self, other):
        return self._entry != PrimedEntry(other)._entry

    def __lt__(self, other):
        return self._entry < PrimedEntry(other)._entry

    def __le__(self, other):
        return self._entry <= PrimedEntry(other)._entry

    def __gt__(self, other):
        return self._entry > PrimedEntry(other)._entry

    def __ge__(self, other):
        return self._entry >= PrimedEntry(other)._entry

    def is_unprimed(self):
        """
        Checks if ``self`` is an unprimed element.

        TEST::

            sage: a = ShiftedPrimedTableau([[1,"2p"]])[0][1]
            sage: a.is_unprimed()
            False
        """
        return is_even(self._entry)

    def is_primed(self):
        """
        Checks if ``self`` is a primed element.

        TEST::

            sage: a = ShiftedPrimedTableau([[1,"2p"]])[0][1]
            sage: a.is_primed()
            True
        """
        return is_odd(self._entry)

    def unprimed(self):
        """
        Unprime ``self`` if it is a primed element.

        TEST::

            sage: a = ShiftedPrimedTableau([[1,"2p"]])[0][1]
            sage: a.unprimed()
            2
        """
        if is_even(self._entry):
            return self
        else:
            return PrimedEntry(double=self._entry + 1)

    def primed(self):
        """
        Prime ``self`` if it is an unprimed element.

        TEST::

            sage: a = ShiftedPrimedTableau([[1,"2p"]])[0][0]
            sage: a.primed()
            1'
        """

        if is_even(self._entry):
            return PrimedEntry(double=self._entry - 1)
        else:
            return self

    def increase_half(self):
        return PrimedEntry(double=self._entry + 1)

    def decrease_half(self):
        return PrimedEntry(double=self._entry - 1)

    def increase_one(self):
        return PrimedEntry(double=self._entry + 2)

    def decrease_one(self):
        return PrimedEntry(double=self._entry - 2)
Last edited 3 years ago by mantepse (previous) (diff)

Changed 3 years ago by mantepse

comment:135 Changed 3 years ago by mantepse

I spent some time abstracting PrimedEntry, but the performance is not really good. Moreover, this raises the question whether the elements of the reading word should be integers or PrimedEntrys.

I attached the file in case it is of any interest.

comment:136 Changed 3 years ago by git

  • Commit changed from fe58704b1a69db9856d397cec4a4a726fbda74c1 to 72d67d79064f7653729e712ef20f25abb6c8a22b

Branch pushed to git repo; I updated commit sha1. New commits:

72d67d7Changed Entry class

comment:137 in reply to: ↑ 134 Changed 3 years ago by kbparamonov

Thank you for helping me with this.

Replying to mantepse:

Travis, isn't that an implementation detail, mostly private? A lot of time already went into this ticket, I would think it might be better to go in for now...

Anyway I also thought a wrapper would be more logical. To be precise, it should be a poset, but that's probably overkill.

EDIT: corrected version.

class PrimedEntry(SageObject):
    """
    The class of entries in shifted primed tableaux.
    """
    def __init__(self, entry=None, double=None):
        """
        Normalize the entry.

        TEST::

            sage: ShiftedPrimedTableau([[1,"2p"]])[0][1]
            2'
            sage: ShiftedPrimedTableau([[1,"2'"]])[0][1]
            2'
            sage: ShiftedPrimedTableau([[1,1.5]])[0][1]
            2'
        """
        # store primed numbers as odd, unprimed numbers as even integers
        if isinstance(entry, self.__class__):
            self._entry = entry._entry
            return

        if double is not None:
            self._entry = Integer(double)
            return

        if isinstance(entry, str):
            if (entry[-1] == "'" or entry[-1] == "p") and entry[:-1].isdigit() is True:
                # Check if an element has "'" or "p" at the end
                self._entry = 2*Integer(entry[:-1]) - 1
                return

        try:
            entry = Rational(entry)
        except (TypeError, ValueError):
            raise ValueError("primed elements have wrong format")
        if entry.denominator() not in (1, 2):
            # Check if an element is a half-integer
            raise ValueError("all numbers must be half-integers")
        self._entry = Integer(2*entry)

    def __hash__(self):
        return self._entry

    def __repr__(self):
        """
        Represent ``self`` as primed or unprimed integer.

        TEST::

            sage: ShiftedPrimedTableau([[1,"2p"]])[0][1]
            2'
        """
        if is_even(self._entry):
            return str(self._entry//2)
        else:
            return str((self._entry+1)//2) + "'"

    def integer(self):
        return self._entry//2

    def __eq__(self, other):
        return self._entry == PrimedEntry(other)._entry

    def __ne__(self, other):
        return self._entry != PrimedEntry(other)._entry

    def __lt__(self, other):
        return self._entry < PrimedEntry(other)._entry

    def __le__(self, other):
        return self._entry <= PrimedEntry(other)._entry

    def __gt__(self, other):
        return self._entry > PrimedEntry(other)._entry

    def __ge__(self, other):
        return self._entry >= PrimedEntry(other)._entry

    def is_unprimed(self):
        """
        Checks if ``self`` is an unprimed element.

        TEST::

            sage: a = ShiftedPrimedTableau([[1,"2p"]])[0][1]
            sage: a.is_unprimed()
            False
        """
        return is_even(self._entry)

    def is_primed(self):
        """
        Checks if ``self`` is a primed element.

        TEST::

            sage: a = ShiftedPrimedTableau([[1,"2p"]])[0][1]
            sage: a.is_primed()
            True
        """
        return is_odd(self._entry)

    def unprimed(self):
        """
        Unprime ``self`` if it is a primed element.

        TEST::

            sage: a = ShiftedPrimedTableau([[1,"2p"]])[0][1]
            sage: a.unprimed()
            2
        """
        if is_even(self._entry):
            return self
        else:
            return PrimedEntry(double=self._entry + 1)

    def primed(self):
        """
        Prime ``self`` if it is an unprimed element.

        TEST::

            sage: a = ShiftedPrimedTableau([[1,"2p"]])[0][0]
            sage: a.primed()
            1'
        """

        if is_even(self._entry):
            return PrimedEntry(double=self._entry - 1)
        else:
            return self

    def increase_half(self):
        return PrimedEntry(double=self._entry + 1)

    def decrease_half(self):
        return PrimedEntry(double=self._entry - 1)

    def increase_one(self):
        return PrimedEntry(double=self._entry + 2)

    def decrease_one(self):
        return PrimedEntry(double=self._entry - 2)

comment:138 Changed 3 years ago by mantepse

Kirill, here are some more trivialities. The first one cuts away a few seconds on my laptop.

Travis, is it good enough now?

diff --git a/src/sage/combinat/shifted_primed_tableau.py b/src/sage/combinat/shifted_primed_tableau.py
index a80a6acaae..be701a06bb 100644
--- a/src/sage/combinat/shifted_primed_tableau.py
+++ b/src/sage/combinat/shifted_primed_tableau.py
@@ -599,7 +599,7 @@ class CrystalElementShiftedPrimedTableau(ShiftedPrimedTableau):
             sage: mat
             [[1, 2', 2, 2], [None, 2, 3', None], [None, None, 3, None]]
         """
-        m = self.shape()[0]
+        m = len(self[0])
         return [[None]*i + list(row) + [None]*(m-i-len(row))
                 for i, row in enumerate(self)]
 
@@ -809,7 +809,7 @@ class CrystalElementShiftedPrimedTableau(ShiftedPrimedTableau):
         element_to_change = None
         count = 0
 
-        for element in read_word[::-1]:
+        for element in reversed(read_word):
             if element[1] == ind:
                 count += 1
             elif count == 0:
@@ -882,7 +882,7 @@ class CrystalElementShiftedPrimedTableau(ShiftedPrimedTableau):
         count = {i: 0 for i in range(max_entry+1)}
         if index_set is None:
             index_set = self.parent().index_set()
-        for l in read_w[::-1]:
+        for l in reversed(read_w):
             count[l] += 1
             if (l-1 in index_set) and (l > 1) and (count[l] > count[l-1]):
                 return False
@@ -2041,7 +2041,7 @@ def _add_strip(sub_tab, full_tab, length):
             if len(sub_tab) < len(full_tab) and len(sub_tab) != 0:
                 plat_list.append(min(sub_tab[-1] + primed_strip[-2] - 1,
                                      full_tab[len(sub_tab)]))
-            for row in range(1, len(sub_tab))[::-1]:
+            for row in reversed(range(1, len(sub_tab))):
                 plat_list.append(
                     min(sub_tab[row-1]+primed_strip[row-1]-1, full_tab[row])
                     - sub_tab[row] - primed_strip[row])

comment:139 Changed 3 years ago by git

  • Commit changed from 72d67d79064f7653729e712ef20f25abb6c8a22b to 7ac6cfb7ce78610bd2fe4f05051b8e3f87e971e1

Branch pushed to git repo; I updated commit sha1. New commits:

7ac6cfbMinor fixes

comment:140 follow-up: Changed 3 years ago by tscrim

  • Milestone changed from sage-8.0 to sage-8.2

I've been making my way through the current code. I think it has significantly improved by having the letter class in readability. Martin and Kirill, thank you for making the changes to the PrimeEntry class.

As part of my review, I have made some changes on my forthcoming push. A good majority of them are trivial changes in formatting and some tricks and micro-optimizations I've learned. Here are some of my more significant changes.

  • I scrubbed all (unnecessary) calls to float and int.
  • I used the fast partition iterator in one place.
  • I removed some (now incorrect) documentation. (I generally do not like implementation details of top-level classes exposed to the user when they do not really "see" those details.)
  • I fixed a bug with weight 0 tableaux.
  • I removed the redundant _element_constructor_s that just made a super call and raised the same error as what the super does. I moved the doctests.
  • Made _reading_word_with_positions an iterator as the list output was never used.
  • I removed the iterator for the crystals because the one from the crystals category is >10x faster:
    sage: SPT = ShiftedPrimedTableaux([3,2], max_entry=3)
    sage: %timeit for x in SPT._old_iter(): pass
    The slowest run took 9.45 times longer than the fastest. This could mean that an intermediate result is being cached.
    100 loops, best of 3: 19.2 ms per loop
    sage: %timeit for x in SPT: pass
    The slowest run took 5.46 times longer than the fastest. This could mean that an intermediate result is being cached.
    1000 loops, best of 3: 1.48 ms per loop
    

However, there are some other things that do need some additional work, but then I think that is the last of it.

  • _contains_tableau_ and _preprocess_ are not special Sage methods, and should not end with a trailing underscore. You also should directly test them as it helps isolate bugs when one gets introduced.
  • For the PrimedEntry class, you should import the class and directly construct the elements instead of indirectly via the tableau. Also, you might want to consider expanding the doc a little more (but not necessary).
  • I noticed that PrimedEntry can sometimes be passed a None, which I do not think should happen. IMO, this is a bug that is hacked around (before, it was handled by Rational(None) being 1 through what is essentially a shortcoming of how Parent.__call__ works). However, it is a technical debt we can take on for now.
  • This is essentially a bug:
    sage: SPT = ShiftedPrimedTableaux(skew=[1])
    sage: SPT.category()
    Category of infinite enumerated sets
    
    because you cannot iterate over this object (currently). In a related issue, I added a test TestSuite(SPT).run() and marked as a known bug. I think you should have the correct categories in this case.

comment:141 Changed 3 years ago by git

  • Commit changed from 7ac6cfb7ce78610bd2fe4f05051b8e3f87e971e1 to d82d038c42d4e785c6c90ebfeb1b382565a7000f

Branch pushed to git repo; I updated commit sha1. New commits:

6ab5d81Merge branch 'public/combinat/primed_tableaux_22921' of git://trac.sagemath.org/sage into public/combinat/primed_tableaux_22921
24e4a34Merge branch 'public/combinat/primed_tableaux_22921' of git://trac.sagemath.org/sage into public/combinat/primed_tableaux_22921
6eabcf4Whitespace, trivial doc fixes, some optimizations, and removing floats.
9ca0634Fixing bug with 0 weight tableaux.
40433ddA little more cleanup.
3466f5cRemoving redundant _element_constructor_'s.
8c155c0Some further simplifications and tweaks.
b60c7fcConvert _reading_word_with_positions to an iterator.
d82d038Use the iterator from the crystals from the category (over 10x faster).

comment:142 Changed 3 years ago by git

  • Commit changed from d82d038c42d4e785c6c90ebfeb1b382565a7000f to 2af9f709afacd0d2abad80b2722eacf1f90d5d76

Branch pushed to git repo; I updated commit sha1. New commits:

2af9f70Minor fixes

comment:143 in reply to: ↑ 140 ; follow-up: Changed 3 years ago by kbparamonov

Alright, I've addressed your comments.

Replying to tscrim:

I've been making my way through the current code. I think it has significantly improved by having the letter class in readability. Martin and Kirill, thank you for making the changes to the PrimeEntry class.

As part of my review, I have made some changes on my forthcoming push. A good majority of them are trivial changes in formatting and some tricks and micro-optimizations I've learned. Here are some of my more significant changes.

  • I scrubbed all (unnecessary) calls to float and int.
  • I used the fast partition iterator in one place.
  • I removed some (now incorrect) documentation. (I generally do not like implementation details of top-level classes exposed to the user when they do not really "see" those details.)
  • I fixed a bug with weight 0 tableaux.
  • I removed the redundant _element_constructor_s that just made a super call and raised the same error as what the super does. I moved the doctests.
  • Made _reading_word_with_positions an iterator as the list output was never used.
  • I removed the iterator for the crystals because the one from the crystals category is >10x faster:
    sage: SPT = ShiftedPrimedTableaux([3,2], max_entry=3)
    sage: %timeit for x in SPT._old_iter(): pass
    The slowest run took 9.45 times longer than the fastest. This could mean that an intermediate result is being cached.
    100 loops, best of 3: 19.2 ms per loop
    sage: %timeit for x in SPT: pass
    The slowest run took 5.46 times longer than the fastest. This could mean that an intermediate result is being cached.
    1000 loops, best of 3: 1.48 ms per loop
    

However, there are some other things that do need some additional work, but then I think that is the last of it.

  • _contains_tableau_ and _preprocess_ are not special Sage methods, and should not end with a trailing underscore. You also should directly test them as it helps isolate bugs when one gets introduced.
  • For the PrimedEntry class, you should import the class and directly construct the elements instead of indirectly via the tableau. Also, you might want to consider expanding the doc a little more (but not necessary).
  • I noticed that PrimedEntry can sometimes be passed a None, which I do not think should happen. IMO, this is a bug that is hacked around (before, it was handled by Rational(None) being 1 through what is essentially a shortcoming of how Parent.__call__ works). However, it is a technical debt we can take on for now.
  • This is essentially a bug:
    sage: SPT = ShiftedPrimedTableaux(skew=[1])
    sage: SPT.category()
    Category of infinite enumerated sets
    
    because you cannot iterate over this object (currently). In a related issue, I added a test TestSuite(SPT).run() and marked as a known bug. I think you should have the correct categories in this case.

comment:144 in reply to: ↑ 143 ; follow-up: Changed 3 years ago by tscrim

Replying to kbparamonov:

Alright, I've addressed your comments.

Replying to tscrim:

  • _contains_tableau_ and _preprocess_ are not special Sage methods, and should not end with a trailing underscore. You also should directly test them as it helps isolate bugs when one gets introduced.

The latter part has not been adequately addressed. In particular, you should have the direct tests for each method testing the key points (ideally every code path).

  • I noticed that PrimedEntry can sometimes be passed a None, which I do not think should happen. IMO, this is a bug that is hacked around (before, it was handled by Rational(None) being 1 through what is essentially a shortcoming of how Parent.__call__ works). However, it is a technical debt we can take on for now.

You should add a test showing that PrimedEntry(None) now raises an error.

I'm also not convinced of having the try-except blocks in the comparisons of PrimedEntry. I think because of how low-level PrimedEntry is, you don't need the extra minor speed hit except for __eq__ and __ne__ (I agree with them raising an error, but the new error message is not really useful IMO).


Sorry, one additional little thing: PrimedEntry.integer, PrimedEntry.decrease_*, and PrimedEntry.increase_* should have at least a one line docstring explaining what they do.

comment:145 Changed 3 years ago by git

  • Commit changed from 2af9f709afacd0d2abad80b2722eacf1f90d5d76 to 6cc7824743549205e46d358cf5b1045ed38eae79

Branch pushed to git repo; I updated commit sha1. New commits:

6cc7824Minor changes in PrimedEntry docs

comment:146 in reply to: ↑ 144 Changed 3 years ago by kbparamonov

Is this what you had in mind?

Replying to tscrim:

Replying to kbparamonov:

Alright, I've addressed your comments.

Replying to tscrim:

  • _contains_tableau_ and _preprocess_ are not special Sage methods, and should not end with a trailing underscore. You also should directly test them as it helps isolate bugs when one gets introduced.

The latter part has not been adequately addressed. In particular, you should have the direct tests for each method testing the key points (ideally every code path).

  • I noticed that PrimedEntry can sometimes be passed a None, which I do not think should happen. IMO, this is a bug that is hacked around (before, it was handled by Rational(None) being 1 through what is essentially a shortcoming of how Parent.__call__ works). However, it is a technical debt we can take on for now.

You should add a test showing that PrimedEntry(None) now raises an error.

I'm also not convinced of having the try-except blocks in the comparisons of PrimedEntry. I think because of how low-level PrimedEntry is, you don't need the extra minor speed hit except for __eq__ and __ne__ (I agree with them raising an error, but the new error message is not really useful IMO).


Sorry, one additional little thing: PrimedEntry.integer, PrimedEntry.decrease_*, and PrimedEntry.increase_* should have at least a one line docstring explaining what they do.

comment:147 Changed 3 years ago by tscrim

Yes, although there are two _contains_tableau that are still not directly doctested. Also, the __ge__ test is actually testing __le__.

comment:148 Changed 3 years ago by git

  • Commit changed from 6cc7824743549205e46d358cf5b1045ed38eae79 to 85353ab927b2b9b01b91aa4a26d5b16936360b9e

Branch pushed to git repo; I updated commit sha1. New commits:

85353abMore minor fixes

comment:149 follow-up: Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Thank you. I am happy and given Martin's comments, I am setting this to a positive review.

comment:150 in reply to: ↑ 149 Changed 3 years ago by kbparamonov

Thank you for reviewing it!

Replying to tscrim:

Thank you. I am happy and given Martin's comments, I am setting this to a positive review.

comment:151 Changed 3 years ago by mantepse

Wonderful! #23896 is then also ready for review! (For some reason, the automerging failed, but I could checkout the ticket without trouble.)

comment:152 Changed 3 years ago by mantepse

  • Status changed from positive_review to needs_work

The patchbot is reporting a failure I overlooked because the doctest is marked "long time":

sage: SPT = ShiftedPrimedTableaux()
sage: TestSuite(SPT).run()
  Failure in _test_eq:
  Traceback (most recent call last):
    File "/home/martin/sage-develop/local/lib/python2.7/site-packages/sage/misc/sage_unittest.py", line 294, in run                                   
      test_method(tester = tester)
    File "sage/structure/element.pyx", line 745, in sage.structure.element.Element._test_eq (build/cythonized/sage/structure/element.c:6812)          
      tester.assertFalse(self == None,
    File "/home/martin/sage-develop/local/lib/python2.7/unittest/case.py", line 416, in assertFalse                                                   
      raise self.failureException(msg)
  AssertionError: broken equality: [] == None                                                                                                         
  ------------------------------------------------------------
  The following tests failed: _test_eq
Failure in _test_elements
The following tests failed: _test_elements

Apparently, it fails because of the following, but I'm not sure:

sage: SPT = ShiftedPrimedTableaux()
sage: SPT.an_element()
[]
sage: SPT.an_element() == None
True
# which is, because
sage: ShiftedPrimedTableau(None)
[]

comment:153 Changed 3 years ago by tscrim

That might be my fault and something I introduced trying to keep the code clean. I am about ready to board a flight to Australia, so I cannot look at it for ~19 hours.

comment:154 follow-up: Changed 3 years ago by mantepse

Quick reply:

    def __classcall_private__(cls, T, skew=None):
...
      if isinstance(T, ShiftedPrimedTableau) and T._skew == skew:
            return T
      if not T or T == [[]]:
            return ShiftedPrimedTableaux(skew=skew)([])

should be

      if T == [] or T == [[]]:
            return ShiftedPrimedTableaux(skew=skew)([])

But the bigger question is: why is not T preferred over T == [] by some people.

The former is impossible to grep for, but still widely used.

comment:155 Changed 3 years ago by mantepse

Please remind me: should the input checking be done in __classcall_private__ and via contains?

For example, I think we should not allow

sage: ShiftedPrimedTableau([[None, None, "1p",None, 2]])
[(None, None, None, 1', 2)]                                                                                                                           

On the other hand:

sage: ShiftedPrimedTableau([tuple([])])

should work, shouldn't it?

comment:156 Changed 3 years ago by mantepse

Another corner case:

sage: ShiftedPrimedTableau([[None]]).pp()
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-2-d20e95e0a9ff> in <module>()
----> 1 ShiftedPrimedTableau([[None]]).pp()

/home/martin/sage-develop/local/lib/python2.7/site-packages/sage/combinat/shifted_primed_tableau.pyc in pp(self)
    494                 .  2'                                                                                                                         
    495         """                                                                                                                                   
--> 496         print(self._repr_diagram())
    497
    498     def _latex_(self):

/home/martin/sage-develop/local/lib/python2.7/site-packages/sage/combinat/shifted_primed_tableau.pyc in _repr_diagram(self)
    324                 .  2'                                                                                                                         
    325         """                                                                                                                                   
--> 326         max_len = len(str(self.max_entry()))+2
    327         return "\n".join([" "*max_len*i + "".join(val)                                                                                        
    328                           for i, val in enumerate(self._repr_tab())])

/home/martin/sage-develop/local/lib/python2.7/site-packages/sage/combinat/shifted_primed_tableau.pyc in max_entry(self)
    528             return 0
    529
--> 530         return max(entry.unprimed() for row in self
    531                    for entry in row if entry is not None)
    532

ValueError: max() arg is an empty sequence
sage: ShiftedPrimedTableau([[None]])
[(None,)]

comment:157 follow-up: Changed 3 years ago by mantepse

Here is a proposal:

diff --git a/src/sage/combinat/shifted_primed_tableau.py b/src/sage/combinat/shifted_primed_tableau.py
index 5fd0c09..be6f46d 100644
--- a/src/sage/combinat/shifted_primed_tableau.py
+++ b/src/sage/combinat/shifted_primed_tableau.py
@@ -108,21 +108,11 @@ class ShiftedPrimedTableau(ClonableArray):
         """
         if isinstance(T, ShiftedPrimedTableau) and T._skew == skew:
             return T
-        if not T or T == [[]]:
-            return ShiftedPrimedTableaux(skew=skew)([])
-        try:
-            entry = T[0][0]
-        except TypeError:
-            raise ValueError("input tableau must be a list of lists")
-
-        if entry is None:
-            skew_ = [row.count(None) for row in T if row[0] is None]
-            if skew is not None:
-                if skew != skew_:
-                    raise ValueError("skew shape does not agree with None entries")
-            else:
-                skew = skew_
-
+        skew_ = Partition([row.count(None) for row in T])
+        if skew_:
+            if skew and Partition(skew) != skew_:
+                raise ValueError("skew shape does not agree with None entries")
+            skew = skew_
         return ShiftedPrimedTableaux(skew=skew)(T)
 
     def __init__(self, parent, T, skew=None, check=True, preprocessed=False):
@@ -163,26 +153,56 @@ class ShiftedPrimedTableau(ClonableArray):
         """
         Preprocessing list ``T`` to initialize the tableau.
 
+        The output is a list of rows as tuples, with explicit
+        ``None``'s to indicate the skew shape, and entries being
+        ``PrimedEntry``s.
+
+        Trailing empty rows are removed.
+
         TESTS::
 
-            sage: ShiftedPrimedTableau._preprocess([["2'", "3p", 3.5]],
-            ....: skew=[1])
+            sage: ShiftedPrimedTableau._preprocess([["2'", "3p", 3.5]], skew=[1])
             [(None, 2', 3', 4')]
+
+            sage: ShiftedPrimedTableau._preprocess([[None, "2'", "3p", 3.5]])
+            [(None, 2', 3', 4')]
+
+            sage: ShiftedPrimedTableau._preprocess([[None]])
+            [(None,)]
+
+            sage: ShiftedPrimedTableau._preprocess([], skew=[2,1])
+            [(None, None), (None,)]
+
+            sage: ShiftedPrimedTableau._preprocess([], skew=[])
+            []
         """
         if isinstance(T, ShiftedPrimedTableau):
             return T
 
-        # Preprocessing list t for primes and other symbols
-        T = [[PrimedEntry(entry) for entry in row if entry is not None]
-             for row in T]
-
-        if skew is not None:
-            T = ([(None,)*skew[i] + tuple(T[i])
-                  for i in range(len(skew))]
-                 + [tuple(T[i]) for i in range(len(skew), len(T))])
-        else:
-            T = [tuple(row) for row in T]
-        return T
+        skew_ = Partition([row.count(None) for row in T])
+        if skew_: # None's are explicit
+            if skew:
+                if Partition(skew) != skew_:
+                    raise ValueError("skew shape does not agree with None entries")
+            T.extend([[]]*(len(skew_)-len(T))) # Warning: these are identical lists
+            for i, s in enumerate(skew_):
+                T[i] = (None,)*s + tuple(PrimedEntry(e) for e in T[i][s:])
+            i += 1
+            T[i:] = [tuple(PrimedEntry(e) for e in row) for row in T[i:]]
+        else: # there are no None's
+            if skew:
+                T.extend([[]]*(len(skew)-len(T))) # Warning: these are identical lists
+                for i, s in enumerate(skew):
+                    T[i] = (None,)*s + tuple(PrimedEntry(e) for e in T[i])
+                i += 1
+                T[i:] = [tuple(PrimedEntry(e) for e in row) for row in T[i:]]
+            else:
+                T = [tuple(PrimedEntry(e) for e in row) for row in T]
+        # remove empty trailing rows
+        for i, row in enumerate(reversed(T)):
+            if row:
+                return T[:-i or None]
+        return []
 
     def check(self):
         """
@@ -354,6 +374,12 @@ class ShiftedPrimedTableau(ClonableArray):
             sage: ascii_art(ShiftedPrimedTableau([]))
             ++
             ++
+
+            sage: ascii_art(ShiftedPrimedTableau([], skew=[1]))
+            +---+
+            | . |
+            +---+
+
         """
         from sage.typeset.ascii_art import AsciiArt
         return AsciiArt(self._ascii_art_table(unicode=False).splitlines())
@@ -383,6 +409,10 @@ class ShiftedPrimedTableau(ClonableArray):
             sage: unicode_art(ShiftedPrimedTableau([]))
             ┌┐
             └┘
+            sage: unicode_art(ShiftedPrimedTableau([], skew=[1]))
+            ┌───┐
+            │ . │
+            └───┘
         """
         from sage.typeset.unicode_art import UnicodeArt
         return UnicodeArt(self._ascii_art_table(unicode=True).splitlines())
@@ -492,6 +522,14 @@ class ShiftedPrimedTableau(ClonableArray):
             sage: s.pp()
              .  .  2' 2  3
                 .  2'
+
+        TESTS::
+
+            sage: ShiftedPrimedTableau([],skew=[1]).pp()
+            .
+
+            sage: ShiftedPrimedTableau([]).pp()
+            <BLANKLINE>
         """
         print(self._repr_diagram())
 
@@ -523,12 +561,19 @@ class ShiftedPrimedTableau(ClonableArray):
             sage: Tab = ShiftedPrimedTableau([(1,1,'2p','3p'),(2,2)])
             sage: Tab.max_entry()
             3
-        """
-        if not self:
-            return 0
 
-        return max(entry.unprimed() for row in self
-                   for entry in row if entry is not None)
+        TESTS::
+
+            sage: Tab = ShiftedPrimedTableau([], skew=[2,1])
+            sage: Tab.max_entry()
+            0
+
+            sage: Tab = ShiftedPrimedTableau([["1p"]], skew=[2,1])
+            sage: Tab.max_entry()
+            1
+        """
+        return max([entry.unprimed() for row in self
+                    for entry in row if entry is not None] + [0])
 
     def shape(self):
         r"""
@@ -2019,4 +2064,3 @@ def _add_strip(sub_tab, full_tab, length):
                                                    k=len(plat_list),
                                                    outer=plat_list):
                 yield list(primed_strip) + list(non_primed_strip)
-
Last edited 3 years ago by mantepse (previous) (diff)

comment:158 Changed 3 years ago by git

  • Commit changed from 85353ab927b2b9b01b91aa4a26d5b16936360b9e to e10754d0b36d1262300b1ac618da5e7f48e28bc7

Branch pushed to git repo; I updated commit sha1. New commits:

e10754dCorner cases

comment:159 in reply to: ↑ 157 Changed 3 years ago by kbparamonov

I agree with the arguments here, though ShiftedPrimedTableau(None) should not work in my opinion. I fixed the _preprocess to deal with empty input tableaux.

Replying to mantepse:

Here is a proposal:

+            sage: ShiftedPrimedTableau._preprocess([[None, "2'", "3p", 3.5]])
+            [(None, 2', 3', 4')]
+
+            sage: ShiftedPrimedTableau._preprocess([[None]])
+            [(None,)]

I don't agree with this test. Since _preprocess is a private function, we can assume all None's in the tableau that get to this point are garbage, and the skew information is passed by skew argument.

comment:160 follow-up: Changed 3 years ago by mantepse

I don't mind - I was only guessing what the responsibilities of _preprocess are. It would be good if you'd clarify by saying what the expected input is.

However, I think that treating [] and [[]] specially, as follows:

        if T == [] or T == [[]]:
             return ShiftedPrimedTableaux(skew=skew)([])

is a very bad idea. For example, it is now unclear whether passing tuples or other iterables would also work - I think they should.

comment:161 Changed 3 years ago by git

  • Commit changed from e10754d0b36d1262300b1ac618da5e7f48e28bc7 to a4d6731c28f45d32464363925fae1152b9ec5233

Branch pushed to git repo; I updated commit sha1. New commits:

a4d6731more input processing

comment:162 in reply to: ↑ 160 Changed 3 years ago by kbparamonov

Fair enough. Let me know if this works.

Replying to mantepse:

I don't mind - I was only guessing what the responsibilities of _preprocess are. It would be good if you'd clarify by saying what the expected input is.

However, I think that treating [] and [[]] specially, as follows:

        if T == [] or T == [[]]:
             return ShiftedPrimedTableaux(skew=skew)([])

is a very bad idea. For example, it is now unclear whether passing tuples or other iterables would also work - I think they should.

comment:163 Changed 3 years ago by mantepse

Sorry, I don't understand. In my proposal, I simply omitted this special case and it worked. Why do you want to treat it specially?

comment:164 in reply to: ↑ 154 Changed 3 years ago by tscrim

Replying to mantepse:

But the bigger question is: why is not T preferred over T == [] by some people.

Because it is both faster and covers a much broader range of inputs uniformly, such as tuples. Although in this case it is a bit too permissive. It probably is better to restrict to lists and tuples. Either that or have an explicit cast to a list/tuple.

The former is impossible to grep for, but still widely used.

If you are greping for it, then you are looking at something far too localized or disregarding the traceback information.

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

comment:165 Changed 3 years ago by git

  • Commit changed from a4d6731c28f45d32464363925fae1152b9ec5233 to bf7e707c6764952aeca7f2b81331cb6b6eed1738

Branch pushed to git repo; I updated commit sha1. New commits:

bf7e707preprocessing for corner cases

comment:166 Changed 3 years ago by mantepse

Works for me! Thanks!

comment:167 Changed 3 years ago by mantepse

  • Status changed from needs_work to needs_review

comment:168 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Since there is a test for these corner cases via the _element_constructor_ and the patchbot are green, back to positive review.

comment:169 Changed 2 years ago by vbraun

  • Branch changed from public/combinat/primed_tableaux_22921 to bf7e707c6764952aeca7f2b81331cb6b6eed1738
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.