Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#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 jdemeyer)

Implements classes and methods related to parking functions

Apply

Attachments (5)

parking_functions-dm.patch (49.0 KB) - added by DorotaMazur 7 years ago.
trac_14086-parking-review-fc.patch (11.3 KB) - added by chapoton 7 years ago.
trac_14086-parking_functions-dm.patch (49.7 KB) - added by DorotaMazur 7 years ago.
trac_14086-parking-review-mz.patch (37.0 KB) - added by zabrocki 7 years ago.
14086-parking_functions_rebased.patch (49.3 KB) - added by jdemeyer 7 years ago.

Download all attachments as: .zip

Change History (27)

Changed 7 years ago by DorotaMazur

comment:1 Changed 7 years ago by zabrocki

  • Cc hivert added
  • Priority changed from major to minor
  • Status changed from new to needs_review
  • Type changed from task to enhancement

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

comment:2 Changed 7 years ago by DorotaMazur

  • Description modified (diff)
  • Summary changed from parking_functions-dm.patch to implements parking functions

comment:3 Changed 7 years ago by DorotaMazur

apply only trac_14086-parking_functions-dm.patch

comment:4 Changed 7 years ago by chapoton

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 chapoton

  • Status changed from needs_review to needs_work
  • Work issues set to import

comment:6 Changed 7 years ago by chapoton

sorry, this was already corrected in the second patch

Last edited 7 years ago by chapoton (previous) (diff)

comment:7 Changed 7 years ago by zabrocki

  • Status changed from needs_work to needs_review

comment:8 Changed 7 years ago by chapoton

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

Last edited 7 years ago by chapoton (previous) (diff)

comment:9 Changed 7 years ago by zabrocki

  • Status changed from needs_review to needs_work

comment:10 Changed 7 years ago by DorotaMazur

  • Status changed from needs_work to needs_review

comment:11 Changed 7 years ago by chapoton

  • 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 DorotaMazur

  • 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 chapoton

Hello,

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

Changed 7 years ago by chapoton

comment:14 Changed 7 years ago by chapoton

  • Description modified (diff)

comment:15 Changed 7 years ago by chapoton

  • Description modified (diff)

comment:16 Changed 7 years ago by zabrocki

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 DorotaMazur

comment:17 in reply to: ↑ description Changed 7 years ago by DorotaMazur

  • Description modified (diff)

Replying to DorotaMazur:

Implements classes and methods related to parking functions

Apply

comment:18 Changed 7 years ago by zabrocki

  • 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 zabrocki

comment:19 Changed 7 years ago by zabrocki

  • Status changed from needs_review to positive_review

I think this is ready.

comment:20 Changed 7 years ago by jdemeyer

  • 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 jdemeyer

  • Merged in set to sage-5.10.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed

Changed 7 years ago by jdemeyer

comment:22 Changed 7 years ago by jdemeyer

  • Dependencies set to #8703, #14433
  • Description modified (diff)
Note: See TracTickets for help on using tickets.