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

Remove deprecated classes ChooseNK and SplitNK #18674

Closed
zabrocki mannequin opened this issue Jun 11, 2015 · 26 comments
Closed

Remove deprecated classes ChooseNK and SplitNK #18674

zabrocki mannequin opened this issue Jun 11, 2015 · 26 comments

Comments

@zabrocki
Copy link
Mannequin

zabrocki mannequin commented Jun 11, 2015

Ticket #10534 deprecated classes ChooseNK and SplitNK and that ticket was merged in Sage 6.3. This ticket is to remove those functions.

Not to be merged before 10 August 2015

CC: @sagetrac-sage-combinat @hivert @videlec @fchapoton @tscrim

Component: combinatorics

Keywords: days65, deprecate

Author: Mike Zabrocki

Branch/Commit: fd5dbe2

Reviewer: Travis Scrimshaw

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

@zabrocki zabrocki mannequin added this to the sage-6.8 milestone Jun 11, 2015
@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Jun 11, 2015

Author: Mike Zabrocki

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Jun 11, 2015

New commits:

63df8b7removed most instances of choose_nk and split_nk, choose_nk.from_rank and choose_nk.rank seem to remain
5258e5esaved the functions rank and from_rank in choose_nk.py
ec6c8c9readded file choose_nk.py
462efd5restore important lines

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Jun 11, 2015

Commit: 462efd5

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Jun 11, 2015

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Jun 11, 2015

comment:2

Question: Should I leave the line

    sage/combinat/choose_nk

in /src/doc/en/reference/combinat/module_list.rst? There are two functions rank and from_rank that are still in that file.

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer removed this from the sage-6.8 milestone Jun 11, 2015
@jdemeyer
Copy link

comment:4

Sage 6.3 was released on 10 August 2014, so this cannot be merged before 10 August 2015.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2015

Changed commit from 462efd5 to 7b35f55

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2015

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

7b35f55restore pickles

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2015

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

39e1b1dMerge branch 'develop' into public/zabrocki/remove/ChooseNK/SplitNK/18674

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2015

Changed commit from 7b35f55 to 39e1b1d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2015

Changed commit from 39e1b1d to 828df24

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2015

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

828df24Merge remote-tracking branch 'origin/public/zabrocki/remove/ChooseNK/SplitNK/18674' into public/zabrocki/remove/ChooseNK/SplitNK/18674

@videlec videlec added this to the sage-6.10 milestone Nov 3, 2015
@videlec videlec removed the pending label Nov 3, 2015
@videlec
Copy link
Contributor

videlec commented Nov 3, 2015

comment:9

If you want a review, you should switch the status to needs review...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2015

Changed commit from 828df24 to 97d0c13

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2015

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

97d0c13remove a few references to choose_nk, move functions in choose_nk.py to combination.py

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 3, 2015

comment:11

I felt it necessary to handle a TODO in choose_nk.py before switching the status which suggested that the choose_nk.rank and choose_nk.from_rank functions should be moved. I put them in combination.py since they were being used there (they are also used in subsets.py). I would not expect that they are being used elsewhere, but I thought it was a good idea to deprecate them as functions in choose_nk.py.

@zabrocki zabrocki mannequin added the s: needs review label Nov 3, 2015
@tscrim
Copy link
Collaborator

tscrim commented Nov 3, 2015

comment:12

You should also change doctest:1: to doctest:...:.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2015

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

ebba4ecfollowing Travis's comment replace :1:

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2015

Changed commit from 97d0c13 to ebba4ec

@videlec
Copy link
Contributor

videlec commented Nov 4, 2015

comment:14

Plenty of doctest failures (see patchbot report)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 5, 2015

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

fd5dbe2change choose_nk to combination

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 5, 2015

Changed commit from ebba4ec to fd5dbe2

@zabrocki zabrocki mannequin added s: needs review and removed s: needs work labels Nov 5, 2015
@tscrim
Copy link
Collaborator

tscrim commented Nov 5, 2015

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Nov 5, 2015

comment:17

LGTM.

@vbraun
Copy link
Member

vbraun commented Nov 5, 2015

Changed branch from public/zabrocki/remove/ChooseNK/SplitNK/18674 to fd5dbe2

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

4 participants