[Sage] #14772: Remove CombinatorialClass from Permutations
Sage
-----------------------------+----------------------------------------------
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: new
Priority: major | Milestone: sage-5.11
Component: combinatorics | Keywords: days49
Work issues: | Report Upstream: N/A
Reviewers: | Authors: Travis Scrimshaw
Merged in: | Dependencies:
Stopgaps: |
-----------------------------+----------------------------------------------
Part of #12913.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica, and MATLAB
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
------------------------------------+---------------------------------------
Changes (by tscrim):
* cc: nthier (added)
* status: new => needs_review
* dependencies: => #8386
Comment:
Minor dependency on #8386 due to use of `Permutation_class`.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:1>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
------------------------------------+---------------------------------------
Changes (by tscrim):
* dependencies: #8386 => #8386 #14519
Comment:
Let's try it with #14519 as a dependency.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:2>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
------------------------------------+---------------------------------------
Comment (by mhansen):
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?
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:3>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
------------------------------------+---------------------------------------
Comment (by tscrim):
I added some (simple) doctests to the `__classcall_private__()` functions.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:4>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
------------------------------------+---------------------------------------
Changes (by tscrim):
* cc: nthier (removed)
* cc: nthiery (added)
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:5>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
------------------------------------+---------------------------------------
* cc: zabrocki (added)
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:6>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
------------------------------------+---------------------------------------
Comment (by tscrim):
All doctest errors should be fixed now.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:7>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
------------------------------------+---------------------------------------
Comment (by darij):
This conflicts with #14882 again. Travis, could you update #14882 please?
Thank you.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:8>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Changes (by tscrim):
* dependencies: #8386 #14519 => #8386 #14519 #14808
Comment:
I put this over #14808, so I'm putting that as a dependency now.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:9>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by darij):
It'll probably need some rebase, though.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:10>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by tscrim):
I already had #14808 applied in the combinat queue.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:11>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by darij):
Here is my queue:
{{{
trac_14117-jordan_normal_form-dg-v3.patch
trac_7983-major_index_and_other_tableau_fixes-dg.patch
trac_14748_deprecationwarning.patch
trac_14136-p_partition_enumerator_folded.patch
trac_14507-tropical_semiring-ts.patch
trac_14775-symmetric_functions_extended-dg.patch
trac_14783-toggles_on_order_ideals-jsdg.patch
trac_14516-crystals_speedup-ts.2.patch
trac_14519-cynthonize_element_wrapper-ts.patch
trac_8386_really_just_moving-fc.patch
trac_8386_big_clean_fc.patch
trac_8386_assert_removal.patch
trac_14808-recoils_of_permutations-dg.patch
trac_14772-remove_cc_permutations-ts.patch
}}}
And here's what I get if I try to push the last one:
{{{
darij@travis-virtualbox:~/sage-5.11.beta3/devel/sage-main$ hg qpush
applying trac_14772-remove_cc_permutations-ts.patch
patching file sage/combinat/permutation.py
Hunk #8 FAILED at 220
Hunk #24 FAILED at 1576
Hunk #25 FAILED at 1598
Hunk #26 FAILED at 1611
Hunk #27 FAILED at 1627
5 out of 43 hunks FAILED -- saving rejects to file
sage/combinat/permutation.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_14772
-remove_cc_permutations-ts.patch
}}}
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:12>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by darij):
But the {{{number_of_inversions(self)}}} docstring is still unfixed, or am
I totally wrong about it?
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:13>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by tscrim):
{{{
#!diff
@@ -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:
}}}
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:14>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by darij):
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.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:15>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by tscrim):
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.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:16>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by tscrim):
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...
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:17>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by darij):
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().
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:18>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by tscrim):
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.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:19>
Sage
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.11
Component: combinatorics | Resolution:
Report Upstream: N/A | Reviewers:
Authors: Travis Scrimshaw | Merged in:
---------------------------------------+------------------------------------
Comment (by tscrim):
Mike, do you think you could do a re-review of this patch? Thanks.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14772#comment:20>