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

A proper random_element() method for PerfectMatchings #9890

Closed
nathanncohen mannequin opened this issue Sep 10, 2010 · 9 comments
Closed

A proper random_element() method for PerfectMatchings #9890

nathanncohen mannequin opened this issue Sep 10, 2010 · 9 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 10, 2010

At the moment, the method random_element() from PerfectMatchings computes a random matching by picking a random integer between 0 and the number of matchings on n elements, then (I presume) enumerating all the matchings according to some ordering until the kth has been computed. This is impressively useless.

sage: %timeit PerfectMatchings(12).random_element()
5 loops, best of 3: 1.5 s per loop

By the way, I was not able to write a method to obtain the list of pairs describing the matching from an PerfectMatching object.. I don't understand how this class is written, and I have no idea why it needs to be so complicated (but it would be nice to add it to this ticket during the review, if someone gets how it works).

The method random_element (and also an_element) both raise an exception when the set of elements is EMPTY. I also fixed the doctests.

(I don't even get why you can build a PerfectMatchings class on an odd number of elements in the first place)

I expect this ticket could be heavily modified during review, but there is a problem with these classes at the moment.

Nathann

CC: @nthiery @hivert

Component: combinatorics

Author: Nathann Cohen

Reviewer: Frédéric Chapoton

Merged: sage-5.10.beta5

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

@nathanncohen nathanncohen mannequin added this to the sage-5.10 milestone Sep 10, 2010
@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Mar 23, 2013

comment:1

I forgot to click on needs_review three years ago T_T

Nathann

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Mar 23, 2013

comment:2

Patch updated ! On the way, I also fixed this "small problem" :

sage: PerfectMatchings(3).an_element()
[(1, 2)]

Nathann

@nathanncohen nathanncohen mannequin added the s: needs review label Mar 23, 2013
@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Mar 23, 2013

Attachment: trac_9890.patch.gz

@fchapoton
Copy link
Contributor

comment:3

hello,

looks good to me.

I have taken the opportunity to make a complete cosmetic clean-up of the file (using pep8 and pyflakes)

if my review patch is ok for you, you can set a positive review

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented May 18, 2013

Author: Nathann Cohen

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented May 18, 2013

comment:4

Attachment: trac_9890_review.patch.gz

I have taken the opportunity to make a complete cosmetic clean-up of the file (using pep8 and pyflakes)

if my review patch is ok for you, you can set a positive review

Okayyyyyyy ! Good to go, then :-)

By the way, how do you use pep8 and pyflakes ? Do you run them externally on files or do you have a way to use them ?

Nathann

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented May 18, 2013

Reviewer: Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:5

Yes, I just run them on the *.py files. Pyflakes is more important, as it finds missing imports and unused variables. Pep8 is much more for the cosmetic, but can check that raise statements are in python3 syntax and find other deprecated syntax.

@jdemeyer
Copy link
Contributor

Merged: sage-5.10.beta5

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