Opened 11 years ago

Closed 11 years ago

#6819 closed enhancement (fixed)

[with patch, positive review] multinomial to accept lists as argument too

Reported by: rishi Owned by: tbd
Priority: major Milestone: sage-4.1.2
Component: algebra Keywords: arithmetic
Cc: Merged in: Sage 4.1.2.alpha0
Authors: Rishikesh Reviewers: Nathann Cohen
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

I have modified multinomial to accept lists as argument too. It makes programming with it much easier

Attachments (3)

12846.patch (1001 bytes) - added by rishi 11 years ago.
multinomial_list.patch (1.5 KB) - added by ncohen 11 years ago.
patch reviewed + some docstrings
trac_6819-reviewer.patch (1.0 KB) - added by mvngu 11 years ago.
ncohen's reviewer patch

Download all attachments as: .zip

Change History (7)

Changed 11 years ago by rishi

comment:1 Changed 11 years ago by rishi

  • Summary changed from multinomial to accept lists as argument too to [with patch; needs review] multinomial to accept lists as argument too

comment:2 follow-up: Changed 11 years ago by ncohen

  • Summary changed from [with patch; needs review] multinomial to accept lists as argument too to [with patch; positive review] multinomial to accept lists as argument too

Seems ok to me ! I Applied it, used it, and did not understand why it was not possible already !

By the way, I added some docstrings to the function, so if you think it is ok.. ;-)

Changed 11 years ago by ncohen

patch reviewed + some docstrings

comment:3 in reply to: ↑ 2 Changed 11 years ago by rishi

Replying to ncohen:

Seems ok to me ! I Applied it, used it, and did not understand why it was not possible already !

By the way, I added some docstrings to the function, so if you think it is ok.. ;-)

Thanks for the docstrings.

Changed 11 years ago by mvngu

ncohen's reviewer patch

comment:4 Changed 11 years ago by mvngu

  • Authors set to Rishikesh
  • Merged in set to Sage 4.1.2.alpha0
  • Resolution set to fixed
  • Reviewers set to Nathann Cohen
  • Status changed from new to closed
  • Summary changed from [with patch; positive review] multinomial to accept lists as argument too to [with patch, positive review] multinomial to accept lists as argument too

The patch multinomial_list.patch contains some badly formatted docstrings. But those shouldn't prevent the patch from being merged. Better to fix those formatting issues in a separate ticket. See #6845 for a follow up to fix this docstring issue.

ncohen -- Your username should be in your patches; it makes it easier to credit you for your contributions. Please also remember to put in a sensible commit message for your patches.

While merging and testing these patches:

  1. 12846.patch -- rishi's contribution
  2. trac_6819-reviewer.patch -- ncohen's contribution

I ran into a doctest failure:

sage -t -long devel/sage-main/sage/misc/getusage.py
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.1/devel/sage-main/sage/misc/getusage.py", line 69:
    sage: get_memory_usage(t)          # amount of memory more than when we defined t.
Expected:
    0.0
Got:
    0.34375
**********************************************************************
1 items had failures:
   1 of   4 in __main__.example_2
***Test Failed*** 1 failures.
For whitespace errors, see the file /scratch/mvngu/release/sage-4.1.1/tmp/.doctest_getusage.py
	 [2.6 s]

This has nothing to do with the above patches. Strangely, it crops up when I run the test on sage.math. But the test passes on mod.math and geom.math. Merged patches in this order:

  1. 12846.patch
  2. trac_6819-reviewer.patch
Note: See TracTickets for help on using tickets.