Opened 11 years ago
Closed 11 years ago
#11411 closed defect (fixed)
some q binomial coefficients should be zero
Reported by: | Frédéric Chapoton | Owned by: | Sage Combinat CC user |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7.2 |
Component: | combinatorics | Keywords: | |
Cc: | Frédéric Chapoton, Sage Combinat CC user | 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 11 years ago by
Attachment: | correction_q_analogues_fc.patch added |
---|
comment:1 Changed 11 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 11 years ago by
Cc: | Sage Combinat CC user added |
---|
comment:4 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:5 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:6 Changed 11 years ago by
Authors: | → Frederic Chapoton |
---|
comment:7 Changed 11 years ago by
Authors: | Frederic Chapoton → Frédéric Chapoton |
---|
comment:8 Changed 11 years ago by
Reviewers: | → Florent Hivert |
---|
comment:9 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:10 follow-up: 12 Changed 11 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:12 Changed 11 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 11 years ago by
Attachment: | trac_11411-q_analogues-review-fh.patch added |
---|
comment:13 follow-up: 14 Changed 11 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 Changed 11 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.
Changed 11 years ago by
Attachment: | trac_11411-correction_q_analogues_fc.patch added |
---|
folded review patch and minor change in qt-catalan
comment:15 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:16 Changed 11 years ago by
Trying to help the build bot to do its job :
Apply trac_11411-correction_q_analogues_fc.patch
comment:19 Changed 11 years ago by
Authors: | Frédéric Chapoton → Frédéric Chapoton |
---|---|
Milestone: | → sage-4.7.1 |
comment:20 Changed 11 years ago by
Milestone: | sage-4.7.1 → sage-4.7.2 |
---|
comment:21 Changed 11 years ago by
Merged in: | → sage-4.7.2.alpha0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → 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 !