fix parent of q-Catalan numbers

[fchapoton]

at n=0 and n=1

plus some pep8 details in the modified file

Component: combinatorics

Author: Frédéric Chapoton

Branch/Commit: 045fbe6

Reviewer: Travis Scrimshaw

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

[fchapoton]

Commit: 3f62bbb

[fchapoton]

New commits:

3f62bbb

fix parent of q_catalan numbers

[fchapoton]

Branch: u/chapoton/29353

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:2

Two questions:

On a similar but technically unrelated issue: What about q_fatorial at n=0? I think that should be special-cased to be q_int(0, q). Similarly for q_binomial when using cyclotomic and n = 0,1.

Is {0, 1}: better (i.e., faster) than [0, 1]:? I have almost always seen the latter in Python code.

Otherwise LGTM.

[fchapoton]

comment:3

Containment in sets seems to be faster (but I am not so sure):

sage: a = {range(200,400)}
sage: b = list(range(200,400))
sage: %timeit 123 in a
The slowest run took 28.00 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 5: 267 ns per loop
sage: %timeit 123 in b
10000 loops, best of 5: 33.2 µs per loop

sage: a = {0,1}
sage: b =[0,1]
sage: %timeit 5 in a
The slowest run took 29.97 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 5: 252 ns per loop
sage: %timeit 5 in b
The slowest run took 29.55 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 5: 307 ns per loop

I will take care of the q_factorial case too.

[sagetrac-git]

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

045fbe6

trac 29353 fix parent of q_factorial(0)

[sagetrac-git]

Changed commit from 3f62bbb to 045fbe6

[fchapoton]

comment:5

I found that page about containment in sets

https://www.geeksforgeeks.org/sets-in-python/

[tscrim]

comment:6

Containment in sets is definitely faster, but you also have to create the set every time. Now that I am in front my computer, I can test this:

sage: def test_set(i):
....:     if i in {0,1}:
....:         return True
....:     return False
sage: def test_list(i):
....:     if i in [0,1]:
....:         return True
....:     return False
sage: %timeit test_set(1)
1000000 loops, best of 5: 372 ns per loop
sage: %timeit test_set(2)
1000000 loops, best of 5: 353 ns per loop
sage: %timeit test_list(1)
1000000 loops, best of 5: 344 ns per loop
sage: %timeit test_list(2)
1000000 loops, best of 5: 346 ns per loop

Surprisingly, tuples are the most optimized:

sage: def test_tuple(i):
....:     if i in (0,1):
....:         return True
....:     return False
sage: %timeit test_tuple(1)
1000000 loops, best of 5: 322 ns per loop
sage: %timeit test_tuple(2)
1000000 loops, best of 5: 328 ns per loop

Not that this matters too much, but just a question I had because it is not something I had seen before. You can set it to a positive review if you don't care enough to change it. I don't care either way.

Sorry, I missed the parent test for q_binomial and the code does cover the (n,k) = (1,1) case by taking the k = min(k, n-k).

Thank you for fixing these bugs.

[fchapoton]

comment:7

ok, setting to positive. Thanks for the review

[vbraun]

Changed branch from u/chapoton/29353 to 045fbe6