octave-bug-tracker
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Octave-bug-tracker] [bug #61565] nchoosek broken for integer inputs


From: Arun Giridhar
Subject: [Octave-bug-tracker] [bug #61565] nchoosek broken for integer inputs
Date: Wed, 1 Dec 2021 16:21:11 -0500 (EST)
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:94.0) Gecko/20100101 Firefox/94.0

Follow-up Comment #7, bug #61565 (project octave):

No, the colon operator has a lower precedence than the arithmetic ones, so the
parentheses don't make a difference.

It turned out the first nchoosek-specific bug was in this line, where k was
being rounded differently when it was an integer (rounded up for odd integers)
compared to a float (always rounded down):

    denom = [(1:k/2) .* (k-1:-1:(k+1)/2) / 2, k];


That line needed to be changed to:

    denom = [(1:(k-1)/2) .* (k-1:-1:(k+1)/2) / 2, k];


The reason this bug was not exposed until now was that the previous line in
nchoosek would trigger an error about ranges, so we didn't get to see this
behavior until the range bugs were fixed first. With the change above,
nchoosek (uint8(10), uint8(5)) evaluates correctly, and it is no longer a
regression from Octave 6.

But that change is not enough, because nchoosek still complains about
mismatched types when given different integer types:

>> nchoosek (uint8(11), int8(5))
error: binary operator '-' not implemented for 'uint8 scalar' by 'int8 scalar'
operations
error: called from
    nchoosek at line 117 column 7


This problem is internal to the integer type system and not because of the
definition of the binomial coefficient. The simplest solution is to cast both
v and k to doubles. Losing precision for v and k above flintmax is not a
problem because nchoosek is required (for Matlab compatibility) to return a
double and any result that exceeds flintmax will trigger a warning anyway.
(Casting v and k to double does not introduce new loss of precision.)

So the two changes to be made are to convert k/2 to (k-1)/2 in the line above,
and to convert v and k to double. Casting to double makes the first change
optional.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?61565>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/




reply via email to

[Prev in Thread] Current Thread [Next in Thread]