#14772: Remove CombinatorialClass from Permutations
------------------------------+--------------------------------------------
Reporter: tscrim | Owner: sage-combinat
Type: | Status: positive_review
enhancement | Milestone: sage-5.11
Priority: major | Resolution:
Component: | Merged in:
combinatorics | Reviewers: Mike Hansen, Darij Grinberg
Keywords: days49 | Work issues:
Authors: Travis | Dependencies: #8386 #14519 #14808
Scrimshaw |
Report Upstream: N/A |
Branch: |
Stopgaps: |
------------------------------+--------------------------------------------
Changes (by tscrim):
* status: needs_review => positive_review
* reviewer: => Mike Hansen, Darij Grinberg
Comment:
Replying to [comment:48 darij]:
> I've done the part of the review I think I was competent enough for. Not
covered are:
>
> -- I hope the {{{__classcall_private__}}} methods are actually doing
what they should; while this is likely given the many doctests, I am not
making any claims about this because I don't know what the heck
{{{_classcall_private__}}} means to begin with.
When doing something like `Permutation([1,2,3])`, what happens is first
`__classcall_private__()` is called which should process the information
and return the correct object (it's super function will call the class's
`__init__()` if called). So in the above example, it creates puts the
permutation in the correct parent class. For `Permutations`, it returns
the correct parent object based upon the input.
> -- Same for {{{__setstate__}}}.
It's used in pickling. This is called when something is trying to be
unpickled, ex. `loads(dumps(obj))`.
> -- {{{sage/combinat/set_partition_ordered.py}}}: I'd like you to remove
that change and qfold the (IMHO more logical) #14883 into this patch
instead (once you've cast an eye on what it does to ensure I did nothing
wrong there).
I can't change this back because I have to make it use the correct class
(because there is no longer a `Permutation_class`). I'll review #14883
however.
> -- A docstring says that {{{__call__}}} is a close variant of
{{{__call__}}}:
> {{{
> def __call__(self, x):
> """
> A close variant of ``__call__`` which just attempts to extend
> the permutation.
> }}}
> This used to be:
> {{{
> def __call__(self, x):
> """
> A close variant of CombinatorialClass.__call__ which just
> attempts to extend the permutation
> }}}
> Since you probably understand this better than I do, it's best if you
fix this docstring.
I've tweaked the docstring a bit so that it's more clear.
> -- Speed might be worth testing after I added those
{{{__contains__}}}es.
Seems okay to me. It's better that it's done correctly anyways.
> Altogether, great job. If you're fine with my review patch and handle
the above five bullet points, feel free to set this to positive review!
Thank you Darij!
--
Ticket URL: <http://trac.sagemath.org/ticket/14772#comment:49>
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 http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/groups/opt_out.
