Opened 9 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 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 chapoton 9 years ago.
trac_11411-q_analogues-review-fh.patch (4.4 KB) - added by hivert 9 years ago.
trac_11411-correction_q_analogues_fc.patch (6.7 KB) - added by chapoton 9 years ago.
folded review patch and minor change in qt-catalan

Download all attachments as: .zip

Change History (24)

Changed 9 years ago by chapoton

comment:1 Changed 9 years ago by chapoton

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by chapoton

  • Description modified (diff)

comment:3 Changed 9 years ago by chapoton

  • Cc sage-combinat added

comment:4 Changed 9 years ago by chapoton

  • Description modified (diff)

comment:5 Changed 9 years ago by chapoton

  • Description modified (diff)

comment:6 Changed 9 years ago by chapoton

  • Authors set to Frederic Chapoton

comment:7 Changed 9 years ago by chapoton

  • Authors changed from Frederic Chapoton to Frédéric Chapoton

comment:8 Changed 9 years ago by hivert

  • Reviewers set to 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 9 years ago by hivert

  • Description modified (diff)

comment:10 follow-up: Changed 9 years ago by 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 9 years ago by chapoton

oops, I was meaning "negative n"

comment:12 in reply to: ↑ 10 Changed 9 years ago by 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 9 years ago by hivert

comment:13 follow-up: Changed 9 years ago by 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 9 years ago by 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 9 years ago by chapoton

folded review patch and minor change in qt-catalan

comment:15 Changed 9 years ago by chapoton

  • Description modified (diff)

comment:16 Changed 9 years ago by chapoton

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 hivert

  • Status changed from needs_review to positive_review

Everything looks good.

comment:18 Changed 8 years ago by chapoton

thanks for the review !

comment:19 Changed 8 years ago by jdemeyer

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton
  • Milestone set to sage-4.7.1

comment:20 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7.1 to sage-4.7.2

comment:21 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.