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:

Status badges

Description (last modified by Frédéric Chapoton)

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)

correction_q_analogues_fc.patch (1.1 KB) - added by Frédéric Chapoton 11 years ago.
trac_11411-q_analogues-review-fh.patch (4.4 KB) - added by Florent Hivert 11 years ago.
trac_11411-correction_q_analogues_fc.patch (6.7 KB) - added by Frédéric Chapoton 11 years ago.
folded review patch and minor change in qt-catalan

Download all attachments as: .zip

Change History (24)

Changed 11 years ago by Frédéric Chapoton

comment:1 Changed 11 years ago by Frédéric Chapoton

Status: newneeds_review

comment:2 Changed 11 years ago by Frédéric Chapoton

Description: modified (diff)

comment:3 Changed 11 years ago by Frédéric Chapoton

Cc: Sage Combinat CC user added

comment:4 Changed 11 years ago by Frédéric Chapoton

Description: modified (diff)

comment:5 Changed 11 years ago by Frédéric Chapoton

Description: modified (diff)

comment:6 Changed 11 years ago by Frédéric Chapoton

Authors: Frederic Chapoton

comment:7 Changed 11 years ago by Frédéric Chapoton

Authors: Frederic ChapotonFrédéric Chapoton

comment:8 Changed 11 years ago by Florent Hivert

Reviewers: Florent Hivert

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 !

comment:9 Changed 11 years ago by Florent Hivert

Description: modified (diff)

comment:10 Changed 11 years ago by Frédéric Chapoton

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 11 years ago by Frédéric Chapoton

oops, I was meaning "negative n"

comment:12 in reply to:  10 Changed 11 years ago by Florent Hivert

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 Florent Hivert

comment:13 Changed 11 years ago by Frédéric Chapoton

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 11 years ago by Florent Hivert

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 Frédéric Chapoton

folded review patch and minor change in qt-catalan

comment:15 Changed 11 years ago by Frédéric Chapoton

Description: modified (diff)

comment:16 Changed 11 years ago by Frédéric Chapoton

Trying to help the build bot to do its job :

Apply trac_11411-correction_q_analogues_fc.patch

comment:17 Changed 11 years ago by Florent Hivert

Status: needs_reviewpositive_review

Everything looks good.

comment:18 Changed 11 years ago by Frédéric Chapoton

thanks for the review !

comment:19 Changed 11 years ago by Jeroen Demeyer

Authors: Frédéric ChapotonFrédéric Chapoton
Milestone: sage-4.7.1

comment:20 Changed 11 years ago by Jeroen Demeyer

Milestone: sage-4.7.1sage-4.7.2

comment:21 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-4.7.2.alpha0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.