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

minor code details in combinat #34019

Closed
fchapoton opened this issue Jun 19, 2022 · 14 comments
Closed

minor code details in combinat #34019

fchapoton opened this issue Jun 19, 2022 · 14 comments

Comments

@fchapoton
Copy link
Contributor

Component: combinatorics

Author: Frédéric Chapoton

Branch/Commit: facaba4

Reviewer: David Coudert

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

@fchapoton fchapoton added this to the sage-9.7 milestone Jun 19, 2022
@fchapoton
Copy link
Contributor Author

New commits:

5e35f94minor details in combinat

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/34019

@fchapoton
Copy link
Contributor Author

Commit: 5e35f94

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2022

Changed commit from 5e35f94 to 632693c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

632693cremove xrange from combinat

@dcoudert
Copy link
Contributor

comment:3

I didn't know that we could check '3 in range(5)`. Good to know.

In src/sage/combinat/designs/orthogonal_arrays_find_recursive.pyx you could add the missing space before #

-    for r in [1] + list(xrange(k+1, n-2)): # as r*1+1+1 <= n and because we need
+    for r in [1] + list(range(k+1, n-2)): # as r*1+1+1 <= n and because we need

In src/sage/combinat/subword_complex.py, isn't it better to turn r to a set ? I don't know if testing membership to a range is faster or not.

-        if not all(i in list(range(len(Q))) for i in F):
+        r = range(len(Q))
+        if not all(i in r for i in F):

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2022

Changed commit from 632693c to facaba4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

facaba4details

@fchapoton
Copy link
Contributor Author

comment:5

voila,j'ai rajouté un ou deux espaces, malgré ma barre d'espacedéfaillante

pour le secondpoint, j'ai laissé tel quel.Jesais pas si ca serait mieux avec set ou carrement avec 0<=i<=k

@dcoudert
Copy link
Contributor

Reviewer: David Coudert

@dcoudert
Copy link
Contributor

comment:6

LGTM.

@fchapoton
Copy link
Contributor Author

comment:7

merci. Pour info:

sage: r = range(40)
sage: s = set(r)
sage: %timeit 34 in r
2.52 µs ± 34.1 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
sage: %timeit 34 in s
197 ns ± 31.8 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: %timeit 0<=34<40
244 ns ± 2.08 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

@dcoudert
Copy link
Contributor

comment:8

It's surprising that set is the fastest here. Interesting.

@vbraun
Copy link
Member

vbraun commented Jun 28, 2022

Changed branch from u/chapoton/34019 to facaba4

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

3 participants