Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get rid of PermutationsNK #16472

Closed
videlec opened this issue Jun 11, 2014 · 21 comments
Closed

get rid of PermutationsNK #16472

videlec opened this issue Jun 11, 2014 · 21 comments

Comments

@videlec
Copy link
Contributor

videlec commented Jun 11, 2014

The class PermutationsNK in sage.combinat.permutation_nk was only used for its iterative property. The latter can be replaced from the permutations in the itertools Python module.

see also: #10534

CC: @sagetrac-sage-combinat @tscrim @nthiery @hivert

Component: combinatorics

Author: Vincent Delecroix

Branch/Commit: c86ef49

Reviewer: Travis Scrimshaw

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

@videlec videlec added this to the sage-6.3 milestone Jun 11, 2014
@videlec
Copy link
Contributor Author

videlec commented Jun 11, 2014

New commits:

4aaf79atrac #16472: get rid of PermutationsNK

@videlec
Copy link
Contributor Author

videlec commented Jun 11, 2014

Branch: u/vdelecroix/16472

@videlec
Copy link
Contributor Author

videlec commented Jun 11, 2014

Commit: 4aaf79a

@videlec

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Jun 12, 2014

Changed commit from 4aaf79a to 3c6d532

@tscrim
Copy link
Collaborator

tscrim commented Jun 12, 2014

comment:3

Hey Vincent, I've added a note about itertools.permutations. If you okay with this change, then positive review.


New commits:

3c6d532Review tweaks.

@tscrim
Copy link
Collaborator

tscrim commented Jun 12, 2014

Changed branch from u/vdelecroix/16472 to public/remove_PermutationsNK-16472

@videlec
Copy link
Contributor Author

videlec commented Jun 12, 2014

comment:4

Hey Travis,

Replying to @tscrim:

Hey Vincent, I've added a note about itertools.permutations. If you okay with this change, then positive review.

Yes. Makes sense!

Thanks

@videlec
Copy link
Contributor Author

videlec commented Jun 12, 2014

Reviewer: Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Jun 15, 2014

comment:5
sage -t --long src/sage/structure/sage_object.pyx
**********************************************************************
File "src/sage/structure/sage_object.pyx", line 1346, in sage.structure.sage_object.unpickle_all
Failed example:
    sage.structure.sage_object.unpickle_all()  # (4s on sage.math, 2011)
Expected:
    doctest:... DeprecationWarning: This class is replaced by Matrix_modn_dense_float/Matrix_modn_dense_double.
    See http://trac.sagemath.org/4260 for details.
    Successfully unpickled ... objects.
    Failed to unpickle 0 objects.
Got:
     * unpickle failure: load('/home/release/.sage/temp/localhost.localdomain/27038/dir_lefRCW//pickle_jar/_class__sage_combinat_permutation_nk_PermutationsNK__.sobj')
    doctest:1: DeprecationWarning: OrderedAlphabet is deprecated; use Alphabet instead.
    See http://trac.sagemath.org/8920 for details.
    doctest:839: DeprecationWarning: This class is replaced by Matrix_modn_dense_float/Matrix_modn_dense_double.
    See http://trac.sagemath.org/4260 for details.
    Failed:
    _class__sage_combinat_permutation_nk_PermutationsNK__.sobj
    ----------------------------------------------------------------------
    ** This error is probably due to an old pickle failing to unpickle.
    ** See sage.structure.sage_object.register_unpickle_override for
    ** how to override the default unpickling methods for (old) pickles.
    ** NOTE: pickles should never be removed from the pickle_jar! 
    ----------------------------------------------------------------------
    Successfully unpickled 585 objects.
    Failed to unpickle 1 objects.
**********************************************************************

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2014

Changed commit from 3c6d532 to c58d158

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2014

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

c58d158trac #16472: dirty hack to make unpickling works!

@videlec
Copy link
Contributor Author

videlec commented Jun 15, 2014

comment:8

Hi Travis,

At least it works! I basically reproduced what I did a long time ago in sage.combinat.alphabet.

Vincent

@tscrim
Copy link
Collaborator

tscrim commented Jun 15, 2014

comment:9

Here's a less hacky way (IMO) to handle the unpickling (it's something that was done for partitions). Pick whichever one you think is best and you can set this to positive review.


New commits:

dd8600dFixed unpickling of old class that probably noone has a pickle of.
645ca8dFix to the fix.

@tscrim
Copy link
Collaborator

tscrim commented Jun 15, 2014

@tscrim
Copy link
Collaborator

tscrim commented Jun 15, 2014

Changed commit from c58d158 to 645ca8d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2014

Changed commit from 645ca8d to c86ef49

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2014

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

c86ef49Fix pickling test

@videlec
Copy link
Contributor Author

videlec commented Jun 15, 2014

comment:11

Hey Travis,

Your version is definitely better. Thanks. But I got

sage -t --long combinat/permutation.py
**********************************************************************
File "combinat/permutation.py", line 7458, in sage.combinat.permutation.PermutationsNK.__setstate__
Failed example:
...
SyntaxError: EOL while scanning string literal

It is because of a backslash that escaped a ". I put a commit for this and now everything is fine so I set to positive review.

Thanks for your help.

Vincent

@tscrim
Copy link
Collaborator

tscrim commented Jun 15, 2014

comment:12

That what I get for testing it in the command line :P. Thanks for catching that.

@vbraun
Copy link
Member

vbraun commented Jun 18, 2014

Changed branch from public/remove_PermutationsNK_pickling-16472 to c86ef49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants