bugGNU Octave - Bugs: bug #64726, betainc: internal integer input...

 
 

bug #64726: betainc: internal integer input logic can be improved

Submitter:  None
Submitted:  Fri 29 Sep 2023 02:29:24 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Other
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 8.3.0
Operating System:  * Any Fixed Release:  8.4.0
Planned Release:  8.4.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 14 Oct 2023 03:30:33 AM UTC, comment #12: 

grafted the change to stable with
http://hg.savannah.gnu.org/hgweb/octave/rev/6a1f89bb969c

updated fixed release.

and of course only now do i see i mis-named the function as betainf in all of my commit messages both times. (function itself is fine though).

Nicholas Jankowski <nrjank>
Group Member
Fri 13 Oct 2023 07:55:56 PM UTC, comment #11: 

i'll look at grafting to stable this weekend and if so will update accordingly.

Nicholas Jankowski <nrjank>
Group Member
Fri 13 Oct 2023 07:33:19 PM UTC, comment #10: 

Looks good to me. Closing as fixed.

@nrjank re comment #8: Are you intending to graft this to stable? If you do, pls change the Fixed Release.

Arun Giridhar <arungiridhar>
Group Member
Fri 29 Sep 2023 11:19:18 PM UTC, comment #9: 

also, if it is later decided this should output floats for ints, and we want to follow the 'if a or b are single, the output should be single' rule used in the rest of the function, that can be done simply by changing lines 126-127:


I = x;
return

to

if (isa (a, "single") || isa (b, "single"))
  I = single (x);
else
  I = double (x);
endif
return


since the int inputs must still be 0 or 1, the early return shortcut still applies.

Nicholas Jankowski <nrjank>
Group Member
Fri 29 Sep 2023 11:10:50 PM UTC, comment #8: 

ok. made some small function changes and pushed
http://hg.savannah.gnu.org/hgweb/octave/rev/43d010974a89

to default.

The main change is adding shortcut paths for integer x and adding x = 0 or 1 cases to the trivial input handling.  also reduces repeat TAIL option checks and adds input validation for nonnumeric inputs, plus some BISTs

(No user visible changes, could push to stable and merge but haven't built stable to run a make check first.)

marking as ready for test.

Nicholas Jankowski <nrjank>
Group Member
Fri 29 Sep 2023 08:27:01 PM UTC, comment #7: 

working on a patch that will add the shortcut path and clean up some of the input validation and checking and add some BISTS.  that shouldn't change any outward behavior. if later we decide output should always be floats instead, that should be an easy change.

Nicholas Jankowski <nrjank>
Group Member
Fri 29 Sep 2023 03:36:40 PM UTC, comment #6: 

i suspect some cases of function-to-function differences are as much a product of attempted compatibility as deliberate design intent. checking that for betainc:

matlab 2023b:

>> betainc([0 1], 1, 2)
ans =
     0     1
>> betainc(int32([0 1]), 1, 2)
Error using betainc
Inputs must be real, full, and double or single.


So no specific compatibility concern here, nor a requirement that we even accept non-float inputs.

So, regarding function intent: it performs the integral from 0 to x of a nontrivial function. But because the matlab and octave definition of the incomplete beta function normalizes it by the full beta function (beta(a,b), same integral from 0 to 1), this changes the output values for the valid int inputs.

x must be [0,1], so the only valid int values for x are 0 and 1.  x=0 reduces the integral from 0 to 0 which is always 0.  x=1 reduces the integral to beta(a,b), and the function to beta(a,b)/beta(a,b) = 1.

So it appears that for x = 0 or 1, always I = x. I can't see any example input that would deviate from this. Separate from the integer class issue, I'd suggest x= 0 or 1 be added to the trivial cases list that currently only looks at a and b.

For integer class input, the multiplications, etc., that would produce odd results for non integer valued a or b would normally have me recommending conversion to double, but those calculations are irrelevant due to the math above.

So I'd recommend current behavior be maintained if input class of x is int, so is output class, and as long as x range is [0,1], then use trivial shortcut path.

Nicholas Jankowski <nrjank>
Group Member
Fri 29 Sep 2023 09:56:26 AM UTC, comment #5: 

There are other examples where an int always results in an int.

class(normpdf(int32(0),int32(2),int32(1)))
--> int32

class(realpow (int32(2), int32(4)))
--> int32

class( perms (int32(2)))
--> int32

But honestly I do not insist albeit the design principle makes sense I think.


Anonymous
Fri 29 Sep 2023 09:19:16 AM UTC, comment #4: 

But other (even non-special) functions give a double when the input is an integer, even if the result is an integer itself


> x = int8(0);
> y = sin(x);
> z = cos(x);
> whos
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
         x           1x1                          1  int8
         y           1x1                          8  double
         z           1x1                          8  double

Total is 3 elements using 17 bytes


Michele Ginesi <m_ginesi>
Fri 29 Sep 2023 09:19:11 AM UTC, comment #3: 

Float means of course octave single data type in this context

Anonymous
Fri 29 Sep 2023 09:12:19 AM UTC, comment #2: 

@type coversion:

A design principle within octave is that the key input variable data type determines the output data type.

So if x is a float one would expect (and gets) a float as output type.

Therefore one might expect an int as output type if x has int as input type.

Note that the numerical result is 0 or 1 so an integer value even when the data type may be different.

Anonymous
Fri 29 Sep 2023 07:55:47 AM UTC, comment #1: 

I think that the line

I = double (x);

was a typo for

x = double (x);


I'm not convinced by initializing I before the conversion. For instance, if x is an integer (let's say int8), I is initialized as an int8 while it should be a double.

I would keep the order as before and only fix the conversion of x

## Convert to floating point if necessary
  if (isinteger (x))
    x = double (x);
  endif
  if (isinteger (a))
    a = double (a);
  endif
  if (isinteger (b))
    b = double (b);
  endif

  ## Initialize output array
  I = zeros (size (x), class (x));


Michele Ginesi <m_ginesi>
Fri 29 Sep 2023 02:29:24 AM UTC, original submission:  

The betainc function betainc(x,a,b, tail) seems to have a minor cosmetic bug concerning the aimed parameter input conversion when x is an integer.


## Convert to floating point if necessary
  if (isinteger (x))
    I = double (x);
  endif
  if (isinteger (a))
    a = double (a);
  endif
  if (isinteger (b))
    b = double (b);
  endif

  ## Initialize output array
  I = zeros (size (x), class (x));



The statement I = double(x) does not make much sense as I is overwritten in all cases a few lines below.



Without spending much time on it:

It appears that the numerical result is still ok. Possible allowed integers (0 <=x <= 1) can only be 0 or 1 and therefore the betainc result is still 0 (when x=0) or 1 (when x=1).

But moving the initialize section BEFORE the possible conversion and setting x to double(x) seems a more logical solution, though.


  ## Initialize output array
  I = zeros (size (x), class (x));

  ## Convert to floating point if necessary
  if (isinteger (x))
    x = double (x);
  endif
  if (isinteger (a))
    a = double (a);
  endif
  if (isinteger (b))
    b = double (b);
  endif


Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by m_ginesi (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-10-14 nrjank Fixed Release9.1.0 8.4.0
        Planned Release9.1.0 8.4.0
    2023-10-13 arungiridhar StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 9.1.0
    2023-09-29 nrjank StatusConfirmed Ready For Test
        Planned ReleaseNone 9.1.0
    2023-09-29 nrjank StatusNone Confirmed
        Operating SystemGNU/Linux Any
    2023-09-29 nrjank SummaryCurrent betainc function has a minor (more cosmetic) bug betainc: internal integer input logic can be improved

    Back to the top

    Powered by Savane 3.13-b921.
    Corresponding source code