Opened 3 years ago
Closed 3 years ago
#29353 closed enhancement (fixed)
fix parent of qCatalan numbers
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  combinatorics  Keywords:  
Cc:  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  045fbe6 (Commits, GitHub, GitLab)  Commit:  045fbe6884eb6d8c1971a2a5666230c8554123bd 
Dependencies:  Stopgaps: 
Description
at n=0 and n=1
plus some pep8 details in the modified file
Change History (8)
comment:1 Changed 3 years ago by
Branch:  → u/chapoton/29353 

Commit:  → 3f62bbb63f5cbcdd77e554d6ed3ced36a0c4583b 
Status:  new → needs_review 
comment:2 Changed 3 years ago by
Reviewers:  → Travis Scrimshaw 

Two questions:
On a similar but technically unrelated issue: What about q_fatorial
at n=0
? I think that should be specialcased 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.
comment:3 Changed 3 years ago by
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.
comment:4 Changed 3 years ago by
Commit:  3f62bbb63f5cbcdd77e554d6ed3ced36a0c4583b → 045fbe6884eb6d8c1971a2a5666230c8554123bd 

Branch pushed to git repo; I updated commit sha1. New commits:
045fbe6  trac 29353 fix parent of q_factorial(0)

comment:6 Changed 3 years ago by
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, nk)
.
Thank you for fixing these bugs.
comment:7 Changed 3 years ago by
Status:  needs_review → positive_review 

ok, setting to positive. Thanks for the review
comment:8 Changed 3 years ago by
Branch:  u/chapoton/29353 → 045fbe6884eb6d8c1971a2a5666230c8554123bd 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
fix parent of q_catalan numbers