#14086 closed enhancement (fixed)
implements parking functions
Reported by: | DorotaMazur | Owned by: | sage-combinat |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.10 |
Component: | combinatorics | Keywords: | Parking Function |
Cc: | zabrocki@…, jean-baptiste.priez@…, hivert | Merged in: | sage-5.10.beta0 |
Authors: | Dorota Mazur | Reviewers: | Mike Zabrocki |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #8703, #14433 | Stopgaps: |
Description (last modified by )
Implements classes and methods related to parking functions
Apply
Attachments (5)
Change History (27)
Changed 7 years ago by
comment:1 Changed 7 years ago by
- Cc hivert added
- Priority changed from major to minor
- Status changed from new to needs_review
- Type changed from task to enhancement
comment:2 Changed 7 years ago by
- Description modified (diff)
- Summary changed from parking_functions-dm.patch to implements parking functions
comment:3 Changed 7 years ago by
apply only trac_14086-parking_functions-dm.patch
comment:4 Changed 7 years ago by
Hello, there seems to be an unused import:
sage/combinat/parking_functions.py:73: 'Composition' imported but unused
comment:5 Changed 7 years ago by
- Status changed from needs_review to needs_work
- Work issues set to import
comment:6 Changed 7 years ago by
sorry, this was already corrected in the second patch
comment:7 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:8 Changed 7 years ago by
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
comment:9 Changed 7 years ago by
- Status changed from needs_review to needs_work
comment:10 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:11 Changed 7 years ago by
- Status changed from needs_review to needs_work
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.
comment:12 Changed 7 years ago by
- Status changed from needs_work to needs_review
I made changes, run the test. I hope it is working now.
comment:13 Changed 7 years ago by
Hello,
things are starting to look good. I upload a first review patch.
Changed 7 years ago by
comment:14 Changed 7 years ago by
- Description modified (diff)
comment:15 Changed 7 years ago by
- Description modified (diff)
comment:16 Changed 7 years ago by
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.
Changed 7 years ago by
comment:17 in reply to: ↑ description Changed 7 years ago by
- Description modified (diff)
Replying to DorotaMazur:
Implements classes and methods related to parking functions
Apply
comment:18 Changed 7 years ago by
- Description modified (diff)
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.
Changed 7 years ago by
comment:19 Changed 7 years ago by
- Status changed from needs_review to positive_review
I think this is ready.
comment:20 Changed 7 years ago by
- Milestone changed from sage-5.9 to sage-5.10
- Reviewers changed from tba to Mike Zabrocki
- Work issues import deleted
comment:21 Changed 7 years ago by
- Merged in set to sage-5.10.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
Changed 7 years ago by
comment:22 Changed 7 years ago by
- Dependencies set to #8703, #14433
- Description modified (diff)
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