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

implements parking functions #14086

Closed
sagetrac-DorotaMazur mannequin opened this issue Feb 9, 2013 · 26 comments
Closed

implements parking functions #14086

sagetrac-DorotaMazur mannequin opened this issue Feb 9, 2013 · 26 comments

Comments

@sagetrac-DorotaMazur
Copy link
Mannequin

sagetrac-DorotaMazur mannequin commented Feb 9, 2013

Implements classes and methods related to parking functions

Apply

Depends on #8703
Depends on #14433

CC: @zabrocki @sagetrac-elixyre @hivert

Component: combinatorics

Keywords: Parking Function

Author: Dorota Mazur

Reviewer: Mike Zabrocki

Merged: sage-5.10.beta0

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

@zabrocki
Copy link
Mannequin

zabrocki mannequin commented Feb 9, 2013

comment:1

Attachment: parking_functions-dm.patch.gz

Florent and Jean-Baptiste,
Can you take a look at this patch and make sure that it will be useful for implementing algebras like PQSym. Do you have any other feedback?
-Mike

@zabrocki zabrocki mannequin added t: enhancement and removed p: major / 3 labels Feb 9, 2013
@sagetrac-DorotaMazur

This comment has been minimized.

@sagetrac-DorotaMazur sagetrac-DorotaMazur mannequin changed the title parking_functions-dm.patch implements parking functions Feb 22, 2013
@sagetrac-DorotaMazur
Copy link
Mannequin Author

sagetrac-DorotaMazur mannequin commented Feb 22, 2013

comment:3

apply only trac_14086-parking_functions-dm.patch

@fchapoton
Copy link
Contributor

comment:4

Hello, there seems to be an unused import:

sage/combinat/parking_functions.py:73: 'Composition' imported but unused

@fchapoton
Copy link
Contributor

Work Issues: import

@fchapoton
Copy link
Contributor

comment:6

sorry, this was already corrected in the second patch

@zabrocki zabrocki mannequin added s: needs review and removed s: needs work labels Mar 6, 2013
@fchapoton
Copy link
Contributor

comment:8

Hello,

you should not use

assert isinstance(n, (Integer, int)) and n >= 0, '%s is not a non-negative integer.' % n 

but rather write a test that raise a ValueError

Similar problem for all the other assert in your patch

@fchapoton
Copy link
Contributor

comment:11

Well, nothing works..

You have made a mistake in changing one of the assert into a test (you forgot to add a not)

Please run sage -t to catch the errors, before posting patches here.

@sagetrac-DorotaMazur
Copy link
Mannequin Author

sagetrac-DorotaMazur mannequin commented Mar 8, 2013

comment:12

I made changes, run the test. I hope it is working now.

@fchapoton
Copy link
Contributor

comment:13

Hello,

things are starting to look good. I upload a first review patch.

@fchapoton
Copy link
Contributor

comment:14

Attachment: trac_14086-parking-review-fc.patch.gz

@fchapoton

This comment has been minimized.

@fchapoton

This comment has been minimized.

@zabrocki
Copy link
Mannequin

zabrocki mannequin commented Mar 14, 2013

comment:16

Dorota, the new version of your patch does not change the /doc/en/reference/combinat/index.rst file to insert the line sage/combinat/parking_functions. I think you will need to make this change.

@sagetrac-DorotaMazur
Copy link
Mannequin Author

sagetrac-DorotaMazur mannequin commented Mar 15, 2013

Attachment: trac_14086-parking_functions-dm.patch.gz

Replying to @sagetrac-DorotaMazur:

Implements classes and methods related to parking functions

Apply

@sagetrac-DorotaMazur

This comment has been minimized.

@zabrocki

This comment has been minimized.

@zabrocki
Copy link
Mannequin

zabrocki mannequin commented Apr 13, 2013

comment:18

The patch trac_14086-parking-review-mz.patch makes a number of minor changes to the documentation and contains all of the changes from trac_14086-parking-review-fc.patch since it needed to rebased against the lastest version.

@zabrocki
Copy link
Mannequin

zabrocki mannequin commented Apr 13, 2013

Attachment: trac_14086-parking-review-mz.patch.gz

@zabrocki
Copy link
Mannequin

zabrocki mannequin commented Apr 13, 2013

comment:19

I think this is ready.

@jdemeyer
Copy link

Changed work issues from import to none

@jdemeyer
Copy link

Reviewer: Mike Zabrocki

@jdemeyer jdemeyer modified the milestones: sage-5.9, sage-5.10 Apr 14, 2013
@jdemeyer
Copy link

Merged: sage-5.10.beta0

@jdemeyer
Copy link

Attachment: 14086-parking_functions_rebased.patch.gz

@jdemeyer
Copy link

Dependencies: #8703, #14433

@jdemeyer

This comment has been minimized.

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

2 participants