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

multinomial to accept lists as argument too #6819

Closed
rishikesha mannequin opened this issue Aug 24, 2009 · 9 comments
Closed

multinomial to accept lists as argument too #6819

rishikesha mannequin opened this issue Aug 24, 2009 · 9 comments

Comments

@rishikesha
Copy link
Mannequin

rishikesha mannequin commented Aug 24, 2009

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

Component: algebra

Keywords: arithmetic

Author: Rishikesh

Reviewer: Nathann Cohen

Merged: Sage 4.1.2.alpha0

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

@rishikesha rishikesha mannequin added this to the sage-4.1.2 milestone Aug 24, 2009
@rishikesha rishikesha mannequin added c: algebra labels Aug 24, 2009
@rishikesha
Copy link
Mannequin Author

rishikesha mannequin commented Aug 24, 2009

comment:1

Attachment: 12846.patch.gz

@rishikesha rishikesha mannequin added the s: needs review label Aug 24, 2009
@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Aug 25, 2009

comment:2

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.. ;-)

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Aug 25, 2009

Attachment: multinomial_list.patch.gz

patch reviewed + some docstrings

@rishikesha
Copy link
Mannequin Author

rishikesha mannequin commented Aug 25, 2009

comment:3

Replying to @nathanncohen:

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.

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Aug 30, 2009

Attachment: trac_6819-reviewer.patch.gz

ncohen's reviewer patch

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Aug 30, 2009

Merged: Sage 4.1.2.alpha0

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Aug 30, 2009

Reviewer: Nathann Cohen

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Aug 30, 2009

Author: Rishikesh

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Aug 30, 2009

comment:4

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

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

0 participants