Remove CombinatorialClass from Permutations

[tscrim]

Part of #12913.

---

Apply: attachment: trac_14772-remove_cc_permutations-ts.patch

Depends on #8386
Depends on #14519
Depends on #14808
Depends on #14143
Depends on #14015
Depends on #14516
Depends on #14863

CC: @sagetrac-sage-combinat @zabrocki @sagetrac-alubovsky @nthiery

Component: combinatorics

Keywords: days49

Author: Travis Scrimshaw

Reviewer: Mike Hansen, Darij Grinberg, Jeff Ferreira

Merged: sage-5.12.beta3

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

[tscrim]

comment:1

Minor dependency on #8386 due to use of Permutation_class.

[tscrim]

Dependencies: #8386

[tscrim]

comment:2

Let's try it with #14519 as a dependency.

[tscrim]

Changed dependencies from #8386 to #8386 #14519

[mwhansen]

comment:3

Looks good to me. The main issue is that there are two classcall_private methods with out tests; however, they are tested in the the doctest for init. I don't think it makes sense to duplicate the tests. Maybe just a note that it is tested via the init doctests?

[tscrim]

comment:4

I added some (simple) doctests to the __classcall_private__() functions.

[tscrim]

comment:7

All doctest errors should be fixed now.

[darijgr]

comment:8

This conflicts with #14808 again. Travis, could you update #14808 please? Thank you.

[tscrim]

comment:9

I put this over #14808, so I'm putting that as a dependency now.

[tscrim]

Changed dependencies from #8386 #14519 to #8386 #14519 #14808

[darijgr]

comment:10

It'll probably need some rebase, though.

[tscrim]

comment:11

I already had #14808 applied in the combinat queue, so it shouldn't need rebasing.

[darijgr]

comment:12

OOPS, you're right. Sorry; I had an obsolete patch in my .hg/patches folder.

[darijgr]

comment:13

But the number_of_inversions(self) docstring is still unfixed, or am I totally wrong about it?

[tscrim]

comment:14

@@ -1542,10 +1561,10 @@ class Permutation_class(CombinatorialObj
 
     def number_of_inversions(self):
         r"""
-        Returns the number of inversions in the permutation p.
-
-        An inversion of a permutation is a pair of elements (p[i],p[j])
-        with i < j and p[i] > p[j].
+        Return the number of inversions in ``self``.
+
+        An inversion of a permutation is a pair of elements `(p_i,p_j)`
+        with `i < j` and `p_i > p_j`.
 
         REFERENCES:
 

[darijgr]

comment:15

I wasn't speaking about the syntax. It should say "a pair of elements (i, j)", not "a pair of elements (p_i, p_j)", so as not to confuse the reader about the output of the inversions() method.

[tscrim]

comment:16

Ah. Fixed. I also added some latex options for permutations and being able to display them as words (which is how I generally look at them, transpositions on positions). I made some other minor changes to other docstrings I noticed, so I apologize in advance if you have to rebase.

[tscrim]

comment:17

With the following patches applied on 5.11.beta3

tscrim@as96:~/sage-5.11.beta3/devel/sage-combinat/sage/combinat$ sage -hg qapplied
trac_14610-LSenergy-ms.patch
trac9107_nesting_nested_classes.patch
trac_9107_fix_cross_reference.patch
trac_13589-categories-c3_under_control-nt.patch
trac_14714_latex_dyckword_fix.patch
trac_14748_deprecationwarning.patch
trac_14748_doctest.patch
trac_14748-review-ts.patch
trac_14787-gyw_stats-bs.patch
trac_14762-fix_ASM_ne-ts.patch
trac_12940_affine_permutations-td.patch
trac_14573-path_realizations-ts.patch
trac_14507-tropical_semiring-ts.patch
trac_11407-list_clone_improve-fh.patch
trac_14516-crystals_speedup-ts.patch
trac_7983-major_index_and_other_tableau_fixes-dg.patch
trac_7983-review-ts.patch
trac_12882-matrix_as_dynkin_diagram-ts.patch
trac_14722-lazy_import_at_startup-nt.patch
trac_8386_really_just_moving-fc.patch
trac_8386_big_clean_fc.patch
trac_8386_assert_removal.patch
trac_14808-recoils_of_permutations-ts.patch
trac_10630-vector_partition-ap.patch
trac_14870-fix_int_mod_QQ-ts.patch
trac_14882-backtrack_longtime-dg.patch
trac_14469_repr_graphics.patch
trac_14519-cynthonize_element_wrapper-ts.patch
trac_12250-ktableaux-as.patch
trac_14776-strong-ktableaux-mz.patch
trac_14772-remove_cc_permutations-ts.patch

I cannot reproduce the doctest failure from the patchbot

tscrim@as96:~/sage-5.11.beta3/devel/sage-combinat/sage/combinat$ sage -tp --long combinat.py
Running doctests with ID 2013-07-13-19-05-23-2a0e8ee8.
Doctesting 1 file using 2 threads.
sage -t --long combinat.py
    [428 tests, 6.61 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 6.9 seconds
    cpu time: 2.4 seconds
    cumulative wall time: 6.6 seconds

Only #12250 and #14476 do not have a positive review and this commutes with those patches. So I don't know why that doctest is failing for the patchbot...

[darijgr]

comment:18

Looks like MRO. The class Arrangements inherits from Permutations. Permutations of a multiset have their own cardinality() method, which gives the (wrong) 1260 answer in this case. The correct 22 answer is obtained by the standard cardinality() method of FiniteEnumeratedSets (?) which just takes the length of the list().

[tscrim]

comment:19

Strange that it wasn't showing up when I was running my doctests (although it did appear when I did it in sage). Typically I get the other behavior. Fixed. Thank Darij.

[tscrim]

comment:20

Mike, do you think you could do a re-review of this patch? Thanks.

[tscrim]

comment:21

Darij, since Mike hasn't responding, do you think you could do the final review of this patch? Thanks.

[darijgr]

comment:22

I've started the review -- but I'm stuck at PermutationOptions now. You have these doctests:

    EXAMPLES::
    
        sage: p213 = Permutation([2,1,3])
        sage: p312 = Permutation([3,1,2])
        sage: PermutationOptions(mult='l2r', display='list')
        sage: po = PermutationOptions()
        sage: po['display']
        'list'
        sage: p213
        [2, 1, 3]
        sage: PermutationOptions(display='cycle')
        sage: p213
        (1,2)
        sage: PermutationOptions(display='singleton')
        sage: p213
        (1,2)(3)
        sage: PermutationOptions(display='list')

But when I try to run them ingame, they don't behave that nicely:

sage:         sage: p213 = Permutation([2,1,3])
sage:         sage: p312 = Permutation([3,1,2])
sage:         sage: PermutationOptions(mult='l2r', display='list')
sage:         sage: po = PermutationOptions()
Current options for permutations
  - display:        list
  - generator_name: s
  - latex:          list
  - mult:           l2r
sage:         sage: po['display']
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-49-b0a2d723e60c> in <module>()
----> 1 po['display']

TypeError: 'NoneType' object has no attribute '__getitem__'

I have a feeling that they are not recognized as doctests due to the weird place they're in... And I don't know what to do about this.

[tscrim]

comment:23

Ack...I forgot to check the global options tests. That syntax is from the old sytle, it should be Permutations.global_options. I'll fix them.

You're also correct in that the doctest framework does not pick up dynamically created docstrings (unfortunately).

Thanks,

Travis

[tscrim]

Changed reviewer from Mike Hansen, Darij Grinberg to Mike Hansen, Darij Grinberg, Jeff Ferreira

[tscrim]

comment:57

Fixed. I think I removed a test from the options after I did a check on the sources. Thanks for catching that.

Doctesting 1 file.
sage -t --long ../doctest/sources.py
    [327 tests, 130.57 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 131.0 seconds
    cpu time: 121.6 seconds
    cumulative wall time: 130.6 seconds

For patchbot:

Apply: trac_14772-remove_cc_permutations-ts.patch

[tscrim]

Attachment: trac_14772-remove_cc_permutations-ts.patch.gz

[tscrim]

comment:58

Trivial rebase over #14863.

For patchbot:

Apply: trac_14772-remove_cc_permutations-ts.patch

[tscrim]

Changed dependencies from #8386 #14519 #14808 #14143 #14015 #14516 to #8386 #14519 #14808 #14143 #14015 #14516 #14863

[zabrocki]

comment:59

There is something wrong !

Before applying this patch:

sage: Permutation([3,2,1])

After applying this patch:

sage: Permutation([3,2,1])
Traceback (most recent call last)
...
TypeError: __init__() got multiple values for keyword argument 'parent'

How can that be an error (and what does that error message tell me)?

[darijgr]

comment:60

Weird, because I'm not getting this error. Can you quote the whole traceback maybe?

[zabrocki]

comment:61

I'm glad that this is not intentional. I was worried that this patch made more significant changes to the use of the Permutation class.

sage: Permutation([3,2,1])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-b3e6779bea6e> in <module>()
----> 1 Permutation([Integer(3),Integer(2),Integer(1)])

/Applications/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (sage/misc/classcall_metaclass.c:1224)()

/Applications/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/combinat/permutation.pyc in __classcall_private__(cls, l, check_input)
    499 
    500         # otherwise, it gets processed by CombinatorialObject's __init__.
--> 501         return Permutations()(l, check_input=check_input)
    502 
    503     def __init__(self, parent, l, check_input=True):

/Applications/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__call__ (sage/structure/parent.c:8154)()

/Applications/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/structure/coerce_maps.so in sage.structure.coerce_maps.DefaultConvertMap_unique._call_with_args (sage/structure/coerce_maps.c:4278)()

/Applications/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/structure/coerce_maps.so in sage.structure.coerce_maps.DefaultConvertMap_unique._call_with_args (sage/structure/coerce_maps.c:4089)()

/Applications/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/categories/sets_cat.pyc in _element_constructor_from_element_class(self, *args, **keywords)
    337                 17
    338             """
--> 339             return self.element_class(parent = self, *args, **keywords)
    340 
    341         def is_parent_of(self, element):

/Applications/sage-5.11.rc0/local/lib/python2.7/site-packages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (sage/misc/classcall_metaclass.c:1243)()

TypeError: __init__() got multiple values for keyword argument 'parent'

[darijgr]

comment:62

I think this is because you don't have #14519 applied. Is that so?

[zabrocki]

comment:63

Yes. That was missing and it seems to work fine. This was my fault. Thanks for helping.

[jdemeyer]

Merged: sage-5.12.beta3