Opened 8 years ago
Closed 8 years ago
#11411 closed defect (fixed)
some q binomial coefficients should be zero
Reported by: | chapoton | Owned by: | sage-combinat |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7.2 |
Component: | combinatorics | Keywords: | |
Cc: | chapoton, sage-combinat | Merged in: | sage-4.7.2.alpha0 |
Authors: | Frédéric Chapoton | Reviewers: | Florent Hivert |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
I have found the following behaviour:
sage: import sage.combinat.q_analogues as qa sage: qa.q_binomial(2,-1) 1/(q^2 + q + 1) sage: binomial(2,-1) 0 sage: qa.q_binomial(2,3) 1/(q^2 + q + 1) sage: binomial(2,3) 0
I think these q-binomials should rather be zero. The q-binomial is based on the q-factorial, where one finds the following behaviour
sage: [qa.q_factorial(-i) for i in range(6)] [1, 1, 1, 1, 1, 1]
This seems to be rather wrong, as the factorial itself is infinite for negative integers.
The patch restricts the q-factorial to nonnegative arguments and defines q-binomials outside of the correct range to be zero.
Apply:
Attachments (3)
Change History (24)
Changed 8 years ago by
comment:1 Changed 8 years ago by
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- Description modified (diff)
comment:3 Changed 8 years ago by
- Cc sage-combinat added
comment:4 Changed 8 years ago by
- Description modified (diff)
comment:5 Changed 8 years ago by
- Description modified (diff)
comment:6 Changed 8 years ago by
comment:7 Changed 8 years ago by
comment:8 Changed 8 years ago by
- Reviewers set to Florent Hivert
comment:9 Changed 8 years ago by
- Description modified (diff)
comment:10 follow-up: ↓ 12 Changed 8 years ago by
This seems to be ok, except the q_binomial procedure, where I have some concern.
I think that, like the usual binomial coefficients, there is a natural extension of the q-binomial coefficients to the case (n,k) with negative k.
sage: binomial(-5,3) -35
Either one has to find this extension somewhere in the litterature, or we should leave the negative n case as NotImplemented?.
Well, I do not know. Maybe what you did is a correct temporary solution.
comment:11 Changed 8 years ago by
oops, I was meaning "negative n"
comment:12 in reply to: ↑ 10 Changed 8 years ago by
Hi Frédéric,
This seems to be ok, except the q_binomial procedure, where I have some concern.
I think that, like the usual binomial coefficients, there is a natural extension of the q-binomial coefficients to the case (n,k) with negative k.
Either one has to find this extension somewhere in the litterature, or we should leave the negative n case as NotImplemented?.
Well, I do not know. Maybe what you did is a correct temporary solution.
A quick search on the web didn't gave me anything. So I have the impression that this negative extension is not quite standard. Anyway, I corrected the error messages.
Changed 8 years ago by
comment:13 follow-up: ↓ 14 Changed 8 years ago by
ok, I see that you have changed the q_binomial. The changes are ok for me.
By the way, one could also check that n is a positive integer in the procedure qt-catalan-number, for coherence. What do you think ? I should have made that myself before.
comment:14 in reply to: ↑ 13 Changed 8 years ago by
By the way, one could also check that n is a positive integer in the procedure qt-catalan-number, for coherence. What do you think ? I should have made that myself before.
That's probably a good idea. My wife is calling for dinner... Can you do it yourself ? Also to be nice to the release manager, it would be better to have only one patch, that is to include my patch into yours. This is called folding a patch: here is the way to go:
hg qgoto Your_patch hg qfold My_review_patch
You can then finish to edit the file as usual. I'll review the final result.
comment:15 Changed 8 years ago by
- Description modified (diff)
comment:16 Changed 8 years ago by
Trying to help the build bot to do its job :
Apply trac_11411-correction_q_analogues_fc.patch
comment:17 Changed 8 years ago by
- Status changed from needs_review to positive_review
Everything looks good.
comment:18 Changed 8 years ago by
thanks for the review !
comment:19 Changed 8 years ago by
- Milestone set to sage-4.7.1
comment:20 Changed 8 years ago by
- Milestone changed from sage-4.7.1 to sage-4.7.2
comment:21 Changed 8 years ago by
- Merged in set to sage-4.7.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
Hi Frédéric,
When you write a patch which fixes a bug, you should add a doctest checking that the bug is indeed fixed, together with the ticket number.
I wrote a small review patch which does that. Your patch is Ok with me but someone must review mine. If you can do that and if my modification are Ok, you can put a positive review on that one !