Implement ordered multiset partitions

[alauve]

An ordered multiset partition C of a multiset X is a list of subsets of X (not multisets), called the blocks of C, whose multi-union is X.

These objects appear in the work of

• [Haglund, Rhoades, Wilson] - arXiv:1509.07058
• [Haglund, Rhoades, Shimozono] - arXiv:1609.07575
• [Benkart, et al] - arXiv:1707.08709v2
• [Lauve, Mastnak] - arXiv:1803.02691

This module provides tools for manipulating ordered multiset partitions.

CC: @tscrim @darijgr @zabrocki @alauve @sagetrac-mshimo @anneschilling @saliola @amypang

Component: combinatorics

Keywords: IMA coding sprint, CHAs, sagedays@icerm

Author: Aaron Lauve, Anne Schilling

Branch: 085a93d

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/25148

[tscrim]

comment:39

Replying to @alauve:

one exception: something that looks like a dictionary but isn't gets treated incorrectly:

sage: P6 = OrderedMultisetPartitions(((1,2), (4,1))); P6.list()
[[{(1,2)}, {(4,1)}], [{(1,2),(4,1)}], [{(4,1)}, {(1,2)}]]

(but I'm not using syntax like P6 anywhere)

I would not worry about that. It is not a dict, but list-like and should behave as such. Basically, I would consider it as garbage-in-garbage-out if you wanted that to behave like a dict.

[tscrim]

comment:40

I don't see why you should have a separate _iterator_weight function. It creates the corresponding parent (and their elements anyways), so it would be much more natural to have it be just that parent class's __iter__ method.

[tscrim]

comment:41

Replying to @alauve:

Anybody, Could you explain why the following two different blocks of code behave differently?

      sage: O1 = OrderedMultisetPartitions(weight=[2,1])
      sage: list(O1) # maximum recursion depth exceded

      sage: O2 = OrderedMultisetPartitions(weight=[2,0,1])
      sage: O2.list()
      [[{1}, {1}, {3}], [{1}, {1,3}], [{1}, {3}, {1}], [{1,3}, {1}], [{3}, {1}, {1}]]
      sage: list(O2)
      [[{1}, {1}, {3}], [{1}, {1,3}], [{1}, {3}, {1}], [{1,3}, {1}], [{3}, {1}, {1}]]

This is misleading. The issue is not the added 0 to the weight, but you must call .list() first. A better example of the underlying issue is:

sage: from sage.combinat.multiset_partition_ordered import *
sage: O = OrderedMultisetPartitions(weight=[2,1])
sage: list(O)
...
RuntimeError: maximum recursion depth exceeded while calling a Python object
sage: O.list()
[[{1}, {1,2}], [{1}, {1}, {2}], [{1,2}, {1}], [{1}, {2}, {1}], [{2}, {1}, {1}]]
sage: list(O)
[[{1}, {1,2}], [{1}, {1}, {2}], [{1,2}, {1}], [{1}, {2}, {1}], [{2}, {1}, {1}]]

What is happening is that list is first making sure that the resulting object is finite by:

1. calling O.__len__,
2. which calls O.cardinality(),
3. which calls len(list(O)),

and hence the infinite recursion. When you call O.list(), it caches the list of objects in O._list and changes the cardinality method to simply call len(O._list). Hence, there is no infinite recursion.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from a7cf678 to 2da6fcf

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

c964cef	Merge branch 'public/combinat/implement-ordered-multiset-partitions-25148' of trac.sagemath.org:sage into public/combinat/implement-ordered-multiset-partitions-25148
c5a44ad	Fixing `list(OrderedMultisetPartitions)` and some other minor fixes.
2da6fcf	Removing `__eq__` because OrderedMultisetPartitions is a subclass of UniqueRepresentation.

[alauve]

comment:43

Replying to @tscrim:

This is misleading. The issue is not the added 0 to the weight, but you must call .list() first.

Yeah, sorry about that. I just wanted to create a new object; knew the 0 had nothing to do with it.

What is happening is that list is first making sure that the resulting object is finite by:

1. calling O.__len__,
2. which calls O.cardinality(),
3. which calls len(list(O)),

and hence the infinite recursion. When you call O.list(), it caches the list of objects in O._list and changes the cardinality method to simply call len(O._list). Hence, there is no infinite recursion.

Awesome. Thanks, Travis! Caching... duh. Good to learn the difference between list(x) and x.list()... so the latter just jumps right into x.__iter__() and crosses its fingers?

Regarding the __eq__ method, I was trying to follow the model in IntegerVectorsConstraints inside integer_vector.py. I don't have a good sense for whether or not all of these

        sage: C = OrderedMultisetPartitions(weight=[2,0,1], length=2); repr(C)
        sage: D1 = OrderedMultisetPartitions(weight={1:2, 3:1}, min_length=2, max_length=2)
        sage: D2 = OrderedMultisetPartitions({1:2, 3:1}, min_length=2, max_length=2)
        sage: D3 = OrderedMultisetPartitions(5, weight={1:2, 3:1}, length=2)
        sage: D4 = OrderedMultisetPartitions([1,3], 3, weight={1:2, 3:1}, length=2)
        sage: D5 = OrderedMultisetPartitions([1,3], 3, size=5, length=2)

should be treated as equal, but they certainly are equal as sets-of-ordered-multiset-partitions.

[tscrim]

comment:44

This fixes the list(O) issue by letting the category framework take care of the cardinality. I added OrderedMultisetPartition/s to the global namespace. I also removed an __eq__ on the parent because they are all subclasses of UniqueRepresentation, which means equality is by id and should never be changed.

[tscrim]

comment:45

Working on a few more changes, hold on.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

6d28f37

Some misc cleanup, fixes, and improvements.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 2da6fcf to 6d28f37

[tscrim]

comment:47

Here is a bunch of general fixes and improvements. I left a few comments in the code of things that I think should be addressed.

I think you are better off implementing the crystal code by adding the corresponding CrystalOfTableaux objects to each MinimajCrystal.Element instance. Since the crystal is essentially a standalone class (you [currently] cannot access it from OrderedMultisetPartitions), it doesn't make sense to constantly go back and forth between the two representations. In many ways, I would argue that it is not worth storing the objects as OMPs, but instead just storing the tableau as a subclass of ElementWrapper, writing a new _repr_/_latex_ that converts to the OMP for printing, and having a to_ordered_multiset_partition for when you really want to consider it as an OMP.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 6d28f37 to 54c824d

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

2ab6575	cleaning up `_repr_` methods; adding tests in __iter__
54c824d	cleaned up code in assorted `__iter__` methods

[alauve]

comment:49

Replying to @tscrim:

Here is a bunch of general fixes and improvements. I left a few comments in the code of things that I think should be addressed.

Thanks, Travis. I'm not sure what "strong code smell" means, but I tried to clean up the __iter__ methods a bit.

.

I think you are better off implementing the crystal code by adding the corresponding CrystalOfTableaux objects to each MinimajCrystal.Element instance. Since the crystal is essentially a standalone class (you [currently] cannot access it from OrderedMultisetPartitions), it doesn't make sense to constantly go back and forth between the two representations. In many ways, I would argue that it is not worth storing the objects as OMPs, but instead just storing the tableau as a subclass of ElementWrapper, writing a new _repr_/_latex_ that converts to the OMP for printing, and having a to_ordered_multiset_partition for when you really want to consider it as an OMP.

Okay. It looks like the only time I'm really using OMP is in .to_tableau() methods within definition of e and f I still need (to be able) to generate all the elements at some point. Would you recommend doing something different than what I've done with this code?:

        self._full_module = OrderedMultisetPartitions(n, ell, length=k)

Also, is it really much more overhead to have elements realized as OMPs instead of tuples of tuples (this is what I'd replace them with, when rewriting self.module_generators within the __init__ method of MinimajCrystal)?

[alauve]

comment:50

A question for Anne:

I identify the module generators for the minimaj crystal by iterating through all crystal elements b and testing:

all(b.e(i) == None for i in range(1,n))

on each one. Do you know of a characterization of the highest weights so we can avoid this slow step?

[tscrim]

comment:51

Replying to @alauve:

Okay. It looks like the only time I'm really using OMP is in .to_tableau() methods within definition of e and f I still need (to be able) to generate all the elements at some point. Would you recommend doing something different than what I've done with this code?:

        self._full_module = OrderedMultisetPartitions(n, ell, length=k)

Also, is it really much more overhead to have elements realized as OMPs instead of tuples of tuples (this is what I'd replace them with, when rewriting self.module_generators within the __init__ method of MinimajCrystal)?

The point mostly is to avoid the round trips through (effectively) CrystalOfTableaux to keep the crystal operators fast. So for that initial step, you can just iterate over the corresponding OMPs and convert them to tableaux. Also, since you are essentially iterating through everything, perhaps a better way would be to do:

self.module_generators = self

and then just define an __iter__ method. The module_generators does not have to be the highest weight elements (e.g., look at the tensor product of crystals).

[alauve]

comment:52

More thoughts on comment:47...

If we store internally as lists of words but display as OMPs, won't this lead to endless bugs and confusion for end-users trying to write elementary code like list(b)?

As it stands, one is best served by storing elements as tableaux (row words, actually) for the e and f methods, but storing as OMPs (or tuples of tuples) for the _repr_ and val methods.

Which of these two pairs are used more often? The to_tableau and from_tableau methods seem pretty cheap. So long as we store things as "tuples of tuples" instead of recreating OMPs all over the place, perhaps that's a good enough savings?

[anneschilling]

comment:53

Replying to @alauve:

A question for Anne:

I identify the module generators for the minimaj crystal by iterating through all crystal elements b and testing:

all(b.e(i) == None for i in range(1,n))

on each one. Do you know of a characterization of the highest weights so we can avoid this slow step?

As Travis wrote below, you can just take all elements in the crystal as its module_generators. The module generators should consist of a set that generates all elements using the f_i and e_i operators, but in the code the set does not have to be minimal. If you inherit from the highest weight crystal category, then you get the highest_weight_vectors method for free.

[alauve]

comment:54

Replying to @anneschilling:

Replying to @alauve: As Travis wrote below, you can just take all elements in the crystal as its module_generators. The module generators should consist of a set that generates all elements using the f_i and e_i operators, but in the code the set does not have to be minimal. If you inherit from the highest weight crystal category, then you get the highest_weight_vectors method for free.

Okay. And for the elements of MinimajCrystal, how do you feel about what is stored internally (versus what is displayed): OMPs, tuples of tuples, list of words, a tensor product in CrystalOfLetters, ...?

[tscrim]

comment:55

Replying to @alauve:

More thoughts on comment:47...

If we store internally as lists of words but display as OMPs, won't this lead to endless bugs and confusion for end-users trying to write elementary code like list(b)?

As it stands, one is best served by storing elements as tableaux (row words, actually) for the e and f methods, but storing as OMPs (or tuples of tuples) for the _repr_ and val methods.

Internally the elements of CrystalOfTableaux are stored as words, which I believe was what your parenthetical is saying. However, if you want things like list(b) to be treated as OMPs, just add an __iter__ of the element class that does return iter(self.to_ordered_multiset_partition()).

Also, some simple doc and an example showing the internal structure can mitigate confusion, but generally I dislike saying anything about the internal structure in public docs. Writing boilerplate functions for a consistent public API is the annoying drawbacks of doing an adapter class, but it does mean it will be quick for its purpose (constructing the crystal).

Which of these two pairs are used more often? The to_tableau and from_tableau methods seem pretty cheap. So long as we store things as "tuples of tuples" instead of recreating OMPs all over the place, perhaps that's a good enough savings?

Compared to the crystal operations themselves, they look very expensive (also considering they are implemented in Python). A good way to actually see what works is to do timings. Compare with crystals.Tableaux, crystals.LSPaths, and crystals.NakajimaMonomials.

sage: def test_iteration(B):
....:     for b in B:
....:         pass

sage: T = crystals.Tableaux(['A',4], shape=[5,3,3,1])
sage: T.cardinality()
1890
sage: %timeit test_iteration(T)
10 loops, best of 3: 75.6 ms per loop

sage: La = RootSystem(['A',4]).weight_space().fundamental_weights()
sage: L = crystals.LSPaths(La[4]+2*La[3]+2*La[1])
sage: %timeit test_iteration(L)
1 loop, best of 3: 2.16 s per loop

sage: La = RootSystem(['A',4]).weight_lattice().fundamental_weights()
sage: M = crystals.NakajimaMonomials(La[4]+2*La[3]+2*La[1])
sage: %timeit test_iteration(M)
1 loop, best of 3: 832 ms per loop

(I really need to get my fast LS path code into Sage...)

[anneschilling]

comment:56

Replying to @tscrim:

Replying to @alauve:

More thoughts on comment:47...

If we store internally as lists of words but display as OMPs, won't this lead to endless bugs and confusion for end-users trying to write elementary code like list(b)?

As it stands, one is best served by storing elements as tableaux (row words, actually) for the e and f methods, but storing as OMPs (or tuples of tuples) for the _repr_ and val methods.

Internally the elements of CrystalOfTableaux are stored as words, which I believe was what your parenthetical is saying. However, if you want things like list(b) to be treated as OMPs, just add an __iter__ of the element class that does return iter(self.to_ordered_multiset_partition()).

Also, some simple doc and an example showing the internal structure can mitigate confusion, but generally I dislike saying anything about the internal structure in public docs. Writing boilerplate functions for a consistent public API is the annoying drawbacks of doing an adapter class, but it does mean it will be quick for its purpose (constructing the crystal).

Which of these two pairs are used more often? The to_tableau and from_tableau methods seem pretty cheap. So long as we store things as "tuples of tuples" instead of recreating OMPs all over the place, perhaps that's a good enough savings?

Compared to the crystal operations themselves, they look very expensive (also considering they are implemented in Python). A good way to actually see what works is to do timings. Compare with crystals.Tableaux, crystals.LSPaths, and crystals.NakajimaMonomials.

sage: def test_iteration(B):
....:     for b in B:
....:         pass

sage: T = crystals.Tableaux(['A',4], shape=[5,3,3,1])
sage: T.cardinality()
1890
sage: %timeit test_iteration(T)
10 loops, best of 3: 75.6 ms per loop

sage: La = RootSystem(['A',4]).weight_space().fundamental_weights()
sage: L = crystals.LSPaths(La[4]+2*La[3]+2*La[1])
sage: %timeit test_iteration(L)
1 loop, best of 3: 2.16 s per loop

sage: La = RootSystem(['A',4]).weight_lattice().fundamental_weights()
sage: M = crystals.NakajimaMonomials(La[4]+2*La[3]+2*La[1])
sage: %timeit test_iteration(M)
1 loop, best of 3: 832 ms per loop

(I really need to get my fast LS path code into Sage...)

I personally think internally you can use whatever is easiest or fastest for the elements. But for the user all elements should be OMPs.

I think there is some bug in the crystal code though since cardinality does not work:

sage: B=crystals.Minimaj(4,8,3)
sage: B.cardinality()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-8-fcb9cf0dbde7> in <module>()
----> 1 B.cardinality()

/Applications/sage/local/lib/python2.7/site-packages/sage/categories/classical_crystals.pyc in cardinality(self)
    428             """
    429             return sum(self.weight_lattice_realization().weyl_dimension(x.weight())
--> 430                        for x in self.highest_weight_vectors())
    431 
    432     class ElementMethods:

/Applications/sage/local/lib/python2.7/site-packages/sage/categories/classical_crystals.pyc in <genexpr>((x,))
    428             """
    429             return sum(self.weight_lattice_realization().weyl_dimension(x.weight())
--> 430                        for x in self.highest_weight_vectors())
    431 
    432     class ElementMethods:

/Applications/sage/local/lib/python2.7/site-packages/sage/combinat/root_system/weight_lattice_realizations.pyc in weyl_dimension(self, highest_weight)
    867                 <... 'sage.rings.integer.Integer'>
    868             """
--> 869             highest_weight = self(highest_weight)
    870             if not highest_weight.is_dominant():
    871                 raise ValueError("the highest weight must be dominant")

/Applications/sage/local/lib/python2.7/site-packages/sage/combinat/root_system/ambient_space.pyc in __call__(self, v)
    196             return self._from_dict(dict((i,K(c)) for i,c in enumerate(v) if c))
    197         else:
--> 198             return CombinatorialFreeModule.__call__(self, v)
    199 
    200 

/Applications/sage/local/lib/python2.7/site-packages/sage/structure/parent.pyx in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9734)()
    918         if mor is not None:
    919             if no_extra_args:
--> 920                 return mor._call_(x)
    921             else:
    922                 return mor._call_with_args(x, args, kwds)

/Applications/sage/local/lib/python2.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4555)()
    143                 print(type(C), C)
    144                 print(type(C._element_constructor), C._element_constructor)
--> 145             raise
    146 
    147     cpdef Element _call_with_args(self, x, args=(), kwds={}):

/Applications/sage/local/lib/python2.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4423)()
    138         cdef Parent C = self._codomain
    139         try:
--> 140             return C._element_constructor(x)
    141         except Exception:
    142             if print_warnings:

/Applications/sage/local/lib/python2.7/site-packages/sage/combinat/free_module.pyc in _element_constructor_(self, x)
    686                 except TypeError:
    687                     pass
--> 688             raise TypeError("do not know how to make x (= %s) an element of self (=%s)"%(x,self))
    689 
    690     def _convert_map_from_(self, S):

TypeError: do not know how to make x (= {1: 4, 2: 2, 3: 2}) an element of self (=Ambient space of the Root system of type ['A', 2])

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

43b233e

changing storage of Minimaj elements to words; cleaning up assorted errors and TODO items

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 54c824d to 43b233e

[alauve]

comment:58

UPDATES:

• I think I've gotten Minimaj working nicely now. I store a crystal element internally as a pair (word, break_points). In Anne's e and f code, she was moving back and forth between OMPs and such pairs (via row-words of skew-tableau), so now we cut out the back-and-forth. Crystal operators are certainly faster now.

• All tests pass throughout the file, as well.

• I am afraid the way I'm bringing in CrystalOfLetters will throw deprecation warnings. How does one import classes from the crystal catalog?

Happy to receive others' comments...

[tscrim]

comment:59

Replying to @alauve:

• I am afraid the way I'm bringing in CrystalOfLetters will throw deprecation warnings. How does one import classes from the crystal catalog?

No it will not. You should import it directly from the file. It will only give a deprecation if you import/use it in the global namespace. Try it.

[anneschilling]

comment:60

Hi Aaron,

Thanks for your work on this. There are some failures for the crystal test suite:

sage: B = crystals.Minimaj(2,3,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
  running ._test_stembridge_local_axioms() . . . 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() . . . 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/classical_crystals.py", line 413, in _test_fast_iter
    SS  = list(Crystals().parent_class.__iter__(self))
  File "sage/sets/recursively_enumerated_set.pyx", line 688, in breadth_first_search_iterator (build/cythonized/sage/sets/recursively_enumerated_set.c:5563)
    known = set(current_level)
  File "sage/structure/element_wrapper.pyx", line 249, in sage.structure.element_wrapper.ElementWrapper.__hash__ (build/cythonized/sage/structure/element_wrapper.c:3569)
    return hash(self.value)
TypeError: unhashable type: 'list'
------------------------------------------------------------
running ._test_new() . . . pass
running ._test_not_implemented_methods() . . . pass
running ._test_pickling() . . . pass
running ._test_some_elements() . . . pass
running ._test_stembridge_local_axioms() . . . pass
The following tests failed: _test_fast_iter

Replying to @alauve:

UPDATES:

• I think I've gotten Minimaj working nicely now. I store a crystal element internally as a pair (word, break_points). In Anne's e and f code, she was moving back and forth between OMPs and such pairs (via row-words of skew-tableau), so now we cut out the back-and-forth. Crystal operators are certainly faster now.

• All tests pass throughout the file, as well.

• I am afraid the way I'm bringing in CrystalOfLetters will throw deprecation warnings. How does one import classes from the crystal catalog?

Happy to receive others' comments...

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

e14c546

switched Minimaj element storage from list to tuple

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 43b233e to e14c546

[alauve]

comment:62

crystal test suite should pass now.

[anneschilling]

comment:63

Replying to @alauve:

UPDATES:

• I think I've gotten Minimaj working nicely now. I store a crystal element internally as a pair (word, break_points). In Anne's e and f code, she was moving back and forth between OMPs and such pairs (via row-words of skew-tableau), so now we cut out the back-and-forth. Crystal operators are certainly faster now.

• All tests pass throughout the file, as well.

• I am afraid the way I'm bringing in CrystalOfLetters will throw deprecation warnings. How does one import classes from the crystal catalog?

Happy to receive others' comments...

I am not sure what you mean by "deprecation warnings" in this context. That is usually what happens when you remove a functionality in sage. You could just import CrystalOfLetters in the method where you use it instead of for the whole file.

[alauve]

comment:64

Replying to @alauve:

UPDATES:

• I am afraid the way I'm bringing in CrystalOfLetters will throw deprecation warnings. How does one import classes from the crystal catalog?

I am not sure what you mean by "deprecation warnings" in this context. That is usually what happens when you remove a functionality in sage. You could just import CrystalOfLetters in the method where you use it instead of for the whole file.

I got the answer I was looking for from Travis. In testing phase, when I was "loading" the .py file into sage, I'd get deprecation warnings on first use of assorted classes. But on a proper rebuild of sage, this didn't happen.

[alauve]

comment:65

Working on documentation today...

Looking through diagram_algebras.py, I notice a few anomalies that make me wonder about conventions.

1/ Regarding references, I see

    REFERENCES:

    - [BH2017]_

(with a corresponding entry in src/doc/en/reference/references/index.rst) as well as

    REFERENCES:

    .. [Naz96] Maxim Nazarov, Young's Orthogonal Form for Brauer's
       Centralizer Algebra. Journal of Algebra 182 (1996), 664--693.

2/ Regarding ":" Sometimes I see

        EXAMPLES:

(see also the "REFERENCES" in Item 1 above) and sometimes

        EXAMPLES::

3/ Regarding uses of TestSuite. Sometimes it appears within the main docstring for a class, and sometimes within the __init__ method for a class. Is the latter preferred, and the former used only when __init__ is not present?

[anneschilling]

comment:66

Replying to @alauve:

Working on documentation today...

Looking through diagram_algebras.py, I notice a few anomalies that make me wonder about conventions.

1/ Regarding references, I see

    REFERENCES:

    - [BH2017]_

(with a corresponding entry in src/doc/en/reference/references/index.rst) as well as

    REFERENCES:

    .. [Naz96] Maxim Nazarov, Young's Orthogonal Form for Brauer's
       Centralizer Algebra. Journal of Algebra 182 (1996), 664--693.

I think both are ok. If the reference is only used locally in the file, you can put it in the file as in the second option. If the reference appears in multiple files, I think it is preferable to put them in the reference file.

2/ Regarding ":" Sometimes I see

        EXAMPLES:

(see also the "REFERENCES" in Item 1 above) and sometimes

        EXAMPLES::

If EXAMPLES is followed by code, then you use EXAMPLES:: followed by indentation. If it is followed by an explanation, then you use EXAMPLES: without indentation. Then you use :: after the text and then the code is indented.

3/ Regarding uses of TestSuite. Sometimes it appears within the main docstring for a class, and sometimes within the __init__ method for a class. Is the latter preferred, and the former used only when __init__ is not present?

Again, I think it does not matter. But if you have an __init__, it is probably best to put it there.

Could you indicate in the crystal code, that you got the initial draft from me?

Thank you!

Anne

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

4055e80	cleaning up documentation
281f72d	cleaning up use of TestSuite
b81b365	swapping order of arguments for MinimajCrystal
4239063	checking that tests pass

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from e14c546 to 4239063

[alauve]

comment:68

Afer the above changes, I think this file is ready for review.

[tscrim]

comment:70

Quick things:

• Not all (which includes private and special) methods have doc(tests).
• Functions with latex in the doc should start r""" (for raw-string, so \ does not become an escape character, i.e., \t in non-raw becomes a tab).

• There is no sorting between integers and strings (in Python3, this is now an error IIRC), which causes this test to fail on the patchbot (even though it might [sometimes] pass on your computer):

  sage -t --long src/sage/combinat/multiset_partition_ordered.py
  **********************************************************************
  File "src/sage/combinat/multiset_partition_ordered.py", line 649, in sage.combinat.multiset_partition_ordered.OrderedMultisetPartition.finer
  Failed example:
    o.finer()
  Expected:
    {[{'b'}, {'a'}, {1}, {1}], [{'a'}, {'b'}, {1}, {1}], [{'a','b'}, {1}, {1}]}
  Got:
    {[{1}, {1}, {'a'}, {'b'}], [{1}, {'a'}, {1}, {'b'}], [{1}, {1,'a'}, {'b'}]}

• Need a blank line here (and some punctuation):

         2. If blocks `B_{i+1}, \ldots, B_k` have been converted to words
            `W_{i+1}, \ldots, W_k`, use the letters in `B_i` to make the unique
            word `W_i` that has a factorization `W_i=(u,v)` satisfying:
  +
            - letters of `u` and `v` appear in increasing order, with `v` possibly empty
            - letters in `vu` appear in increasing order
            - ``v[-1]`` is the largest letter `a \in B_i` satisfying ``a <= W_{i+1}[0]``

  Similar for the .. NOTE:: block in the crystal element class.

• References should also have 4 number years (and IMO, I prefer them to be in the master ref file, helps reduce the risk of duplicate references).
• You should not raise an AssertionError (these are only for real bugs, not bad input).
• if hasattr(x, "value"): is not a good check. In fact, the coercion framework makes it so that _element_constructor_ is never called when doing P(x) if the parent of x is P. Thus, you can simply remove this.
• For your MinimajCrystal.__contains__, a better check would be isinstance(x, Element).
• It should be .. NOTE::, not .. NOTES: and what follows should be indented.

• IMO, I would add an AUTHORS: block to MinimajCrystal saying something like

  AUTHORS:
  
  - Anne Schilling: initial draft
  - Aaron Lauve: changed to use `ElementWrapper`

• I still think you should use the CrystalOfTableaux code rather than manually working with a tensor product of letters as it is essentially code duplication.
• `ZZ` -> `\ZZ`
• In terms of speed, it is much better to use Python's frozenset instead of Sage's Set.

I will probably have some more comments later.

[tscrim]

Changed author from Aaron Lauve to Aaron Lauve, Anne Schilling

[tscrim]

Reviewer: Travis Scrimshaw

[alauve]

comment:71

Thanks, Travis. Informative and helpful comments, all.

I'll start on them asap... feel free to hold off on your "more comments later" until after the next iteration.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

0c5aa94	updating references, correcting raw-string usage in docs
4ccdd93	adding missing docstrings
b144c19	removing AssertionError calls
dad7e7b	moving from Set to frozenset; zapping doctest errors that creeped in from earlier changes
306b3fe	modified subset method to behave the way expected from `__init__` method in CombinatorialFreeModule

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 4239063 to 306b3fe

[alauve]

comment:73

Travis,

I finished implementing most of your suggested changes (plus an additional one).

I'm not quite sure what to do about your CrystalsOfTableaux suggestion:

• the elements are not individual instances crystals.Tableaux(['A', n-1], shape=??) but rather tensor products of these (so what code are we duplicating?)
• the constituent shapes ("??" above) are not easily predetermined and are not uniform across all ordered multiset partitions of order d and length k

I have left that bit of code alone for now.

[anneschilling]

comment:74

Sorry, I am currently at a conference, so I am a bit slow. But it seems that there are a lot of docstrings missing:

sage -coverage multiset_partition_ordered.py 
------------------------------------------------------------------------
SCORE multiset_partition_ordered.py: 57.4% (62 of 108)

Missing doctests:
     * line 252: def _repr_(self)
     * line 258: def _repr_normal(self)
     * line 326: def __ne__(self, y)
     * line 413: def letters(self)
     * line 1443: def __init__(self, is_finite=None, **constraints)
     * line 1507: def _repr_(self)
     * line 1513: def _constraint_repr_(self, cdict=None)
     * line 1596: def _has_valid_blocks(self, x)
     * line 1607: def _satisfies_constraints(self, x)
     * line 1693: def an_element(self)
     * line 1819: def _repr_(self)
     * line 1897: def _has_valid_blocks(self, x)
     * line 1909: def cardinality(self)
     * line 1930: def an_element(self)
     * line 1969: def __iter__(self)
     * line 1998: def _repr_(self)
     * line 2007: def _has_valid_blocks(self, x)
     * line 2048: def _has_valid_blocks(self, x)
     * line 2058: def cardinality(self)
     * line 2074: def an_element(self)
     * line 2125: def __iter__(self)
     * line 2156: def _repr_(self)
     * line 2166: def _has_valid_blocks(self, x)
     * line 2214: def _has_valid_blocks(self, x)
     * line 2226: def an_element(self)
     * line 2263: def __iter__(self)
     * line 2325: def _repr_(self)
     * line 2336: def _has_valid_blocks(self, x)
     * line 2343: def an_element(self)
     * line 2366: def _get_multiset(co)
     * line 2372: def _get_weight(lst)
     * line 2381: def _union_of_sets(list_of_sets)
     * line 2387: def _concatenate(list_of_iters)
     * line 2396: def _is_finite(constraints)
     * line 2407: def _base_iterator(constraints)
     * line 2577: def _descents(w)
     * line 2583: def _break_at_descents(alpha, weak=True)
     * line 2613: def _refine_block(S, strong=False)
     * line 2641: def _is_initial_segment(lst)
     * line 2647: def _split_block(S, k=2)
     * line 2779: def an_element(self)
     * line 2788: def _element_constructor_(self, x)
     * line 2798: def __contains__(self, x)
     * line 2873: def _repr_(self)
     * line 2879: def __iter__(self)
     * line 2885: def _minimaj_blocks_from_word_pair(self)

Possibly wrong (function name doesn't occur in doctests):
     * line 2269: def cardinality(self)

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

c68b518

updated minimaj crystal reference

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 306b3fe to c68b518

[alauve]

comment:76

I see. One needs EXAMPLES or TESTS for every function, not just docstring? Okay, I'll try to add some this morning.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

4af474f

mostly adding doc tests and examples; improved a few methods

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from c68b518 to 4af474f

[alauve]

comment:78

Well, sage -coverage gives me a grade of 90.7% now instead of 57.4%. I'll get back to example/test writing soon.