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 CombinatorialClass from Compositions #14063

Closed
tscrim opened this issue Feb 5, 2013 · 17 comments
Closed

Remove CombinatorialClass from Compositions #14063

tscrim opened this issue Feb 5, 2013 · 17 comments

Comments

@tscrim
Copy link
Collaborator

tscrim commented Feb 5, 2013

Part of #12913.

Depends on #14065

CC: @sagetrac-sage-combinat @nthiery

Component: combinatorics

Keywords: deprecation

Author: Travis Scrimshaw

Reviewer: Vincent Delecroix

Merged: sage-5.8.beta2

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

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 6, 2013

Dependencies: #14065

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 6, 2013

Author: Travis Scrimshaw

@videlec
Copy link
Contributor

videlec commented Feb 15, 2013

comment:2

Hi,

  • composition_from_subset must be renamed to from_subset
  • You may remove the few trailing whitespaces that remain.

The rest looks good.

Vincent

@videlec
Copy link
Contributor

videlec commented Feb 15, 2013

comment:3

Comments on the doc in composition.py

  • line 786: list(Compositions(4)) may be replaced with Compositions(4).list() (idem lines 877, 882, 927)
  • line 844: there is a strange sharp at the end of the line
  • line 859: a space is missing

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 15, 2013

comment:4

Made fixes based on your comments.

Thanks,

Travis

@videlec
Copy link
Contributor

videlec commented Feb 15, 2013

comment:5

Great job !

Vincent

@jdemeyer
Copy link
Contributor

Reviewer: Vincent Delecroix

@jdemeyer jdemeyer modified the milestones: sage-5.7, sage-5.8 Feb 16, 2013
@jdemeyer
Copy link
Contributor

comment:7

How was this patch file created, as some headers are missing? You should use hg export [tip].

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 19, 2013

comment:8

Forgot to export, sorry about that. Fixed.

@jdemeyer
Copy link
Contributor

Merged: sage-5.8.beta1

@jdemeyer
Copy link
Contributor

comment:10

The LaTeX documentation doesn't build because you use backslashes inside the

def cardinality(self):
"""
"""

docstring instead of

def cardinality(self):
r"""
"""

@jdemeyer
Copy link
Contributor

Changed merged from sage-5.8.beta1 to none

@jdemeyer jdemeyer reopened this Feb 23, 2013
@tscrim
Copy link
Collaborator Author

tscrim commented Feb 23, 2013

comment:11

Made the fixes (I put it in a few other places as well). I'll double/triple-check over the compiled docbuild once it finishes the rebuild.

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 23, 2013

comment:12

I also fixed a broken link. Jeroen, would it be okay if I set this back to positive review?

Sorry about this,

Travis

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 23, 2013

Fixed docstrings

@tscrim
Copy link
Collaborator Author

tscrim commented Feb 23, 2013

comment:13

Attachment: trac_14063-remove_cc_compositions-ts.patch.gz

I'm setting this back to positive review...I hope that's okay...

@jdemeyer
Copy link
Contributor

Merged: sage-5.8.beta2

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