#20555: descents for Permutations : cleanup
-------------------------+-------------------------------------------------
Reporter: | Owner:
chapoton | Status: needs_review
Type: | Milestone: sage-7.2
enhancement | Resolution:
Priority: major | Merged in:
Component: | Reviewers: Travis Scrimshaw, Christian Stump
combinatorics | Work issues:
Keywords: | Commit:
permutation | 0e1c23bc2d7326980c1970e7fc0fcb379a629f78
Authors: | Stopgaps:
Frédéric Chapoton |
Report Upstream: N/A |
Branch: |
u/chapoton/20555 |
Dependencies: |
-------------------------+-------------------------------------------------
Comment (by tscrim):
Sorry for the delay in my comments. Here they are:
- I think it is better to use the more explicit `tester.assertEqual` and
`tester.assertNotIn` (they have better default error messages).
- You also need to have some sort of warning message saying that the
behavior of `ParkingFunction.ides` has changed.
- Some docstring changes:
{{{#!diff
INPUT:
- - ``final_descent`` -- optional boolean (default ``False``)
- If ``True``, the last position of a non-empty
- permutation is also considered as a descent.
+ - ``final_descent`` -- boolean (default ``False``);
+ if ``True``, the last position of a non-empty
+ permutation is also considered as a descent
- - ``side`` -- optional, ``'right'`` (default) or ``'left'``
- If ``'left'``, return the descents of the inverse permutation.
+ - ``side`` -- ``'right'`` (default) or ``'left'``;
+ if ``'left'``, return the descents of the inverse permutation
- - ``positive`` -- optional boolean (default ``False``)
- If ``True``, return the positions that are not descents.
+ - ``positive`` -- boolean (default ``False``);
+ if ``True``, return the positions that are not descents
- - ``from_zero`` -- optional boolean (default ``True``)
- If ``False``, return the positions starting from `1`
+ - ``from_zero`` -- boolean (default ``True``);
+ if ``False``, return the positions starting from `1`
}}}
Similarly for `idescents`.
- Your deprecation message does not match the docstring. If `from_zero` is
going to be removed, we should explicitly state in that in the docstring.
Also the default behavior either will change or this argument should not
be deprecated.
As a counterpoint, the (very) good reasons that descents start at 0 is
because Python uses 0-based indexing:
{{{
sage: p = Permutation([5,3,2,4,1])
sage: p.descents()
[0, 1, 3]
sage: p[0]
5
}}}
This would break this behavior with the current branch, and actually,
thinking a bit more now about it, we should add some documentation to this
point.
(I am pretty sure the position of the descent must correspond to the
position of the larger value for the related math to work, but it's been
awhile since I've thought about these things.)
However, I think with sufficient advertising on sage-devel and this
deprecation, the migration should be smooth.
--
Ticket URL: <http://trac.sagemath.org/ticket/20555#comment:26>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
