bugGNU Octave - Bugs: bug #63089, dec2bin gives wrong answer for...

 
 

bug #63089: dec2bin gives wrong answer for negative int16

Submitter:  Mayeul C. <mayeulc>
Submitted:  Wed 21 Sep 2022 02:51:57 PM UTC
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Ready For Test Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

Sun 25 Sep 2022 10:34:01 AM UTC, comment #9: 

Thanks for checking with Matlab. I've pushed changes to both dec2bin and dec2hex here: https://hg.savannah.gnu.org/hgweb/octave/rev/dd6b37f67db2

Marking as Ready For Test.

Arun Giridhar <arungiridhar>
Project Member
Sat 24 Sep 2022 09:02:11 PM UTC, comment #8: 

R2022b:

dec2bin(int16(-4200),5): 1110111110011000

dec2bin(-3): 11111101

dec2bin(-4200): 1110111110011000

dec2bin(intmin("int64")): 1000000000000000000000000000000000000000000000000000000000000000

A.R. Burgers <arb>
Sat 24 Sep 2022 08:51:02 PM UTC, comment #7: 

There's so much commonality between dec2bin and dec2hex that I rewrote dec2hex to make use of the patched dec2bin, instead of repeating all the preparation code. Now dec2hex also accepts 64-bit negative input.

Patch attached for dec2hex.

(file #53751)

Arun Giridhar <arungiridhar>
Project Member
Sat 24 Sep 2022 05:45:05 PM UTC, comment #6: 

Improved patch attached (v2).

I activated several BISTs that had been commented out. All BISTs now pass. I've fixed them to expect 64-bit strings instead of 53-bit ones, and to allow numbers down to -2^63 without expecting an error.

The practice of left-padding with ones for negative inputs is standard two's complement notation, but the practice of further left-padding that with zeroes is not mathematically sound at all. Is Matlab still doing that or have they fixed it to consistently left-pad with ones for all negative inputs? In any case, I've added a note to the docstring explaining what to expect.

Since the domain of valid inputs has changed, should we mention this in NEWS? If so, is it going into 7.3 or 8?

(file #53748)

Arun Giridhar <arungiridhar>
Project Member
Sat 24 Sep 2022 03:17:39 PM UTC, comment #5: 

I am attaching a WIP patch for testing. It is vectorized (no loops) and can accommodate negative numbers below flintmax to the full range of int64 for signed and uint64 for unsigned. It passes all BISTs except for these:

dec2bin (-2^31 -1)
dec2bin (-2^52)
error <negative inputs cannot be less than> dec2bin (- flintmax ())

In the first two cases, the expected result was a 53-bit string but the patched version returns a 64-bit string with more leading 1s, which on reflection I think is an improvement since the other conversions of negative numbers when length is not specified return 8, 16, or 32 bits, so returning a bit string with 64 bits is more consistent than 53 bits.

The last failing BIST is because the patched version can handle negative numbers below flintmax, so the expected error does not happen.

The trimming-to-length part is subjective for negative numbers, because one can either return the minimum number of bits plus a leading "1", or one can return a fixed power-of-2 number of bits with several leading "1"s. Both choices can be defended. The patch at present does the second case, which is similar what C++ does when casting signed to unsigned, returning e.g. 32 bits or 64 bits but not something in between:

  int32_t c = -123456789;
  int64_t d = -123456789;
  std::cout << uint32_t (c) << ' ' << uint64_t (d) << '\n';

Please test against Matlab for the following cases:

dec2bin (-3)     # should return 8 bits
dec2bin (-4200)  # should return 16 bits
dec2bin (int16(-4200), 5)  # should hopefully still return 16 bits, not just 13 bits
dec2bin (intmin ("int64")) # should return one 1 followed by 63 zeroes.

(file #53747)

Arun Giridhar <arungiridhar>
Project Member
Fri 23 Sep 2022 12:28:44 PM UTC, comment #4: 

Also, are there specific function calls we should check against Matlab? If you provide examples someone can test the and provide the output from a current version.

Nicholas Jankowski <nrjank>
Project Member
Fri 23 Sep 2022 08:15:37 AM UTC, comment #3: 

Could you please provide your proposed changes in form of a Mercurial (or Git) patch relative to the current head of the Octave repository?

Markus M├╝tzel <mmuetzel>
Project Member
Wed 21 Sep 2022 07:46:36 PM UTC, comment #2: 

Affected file is https://hg.savannah.gnu.org/hgweb/octave/file/b542b88ad3b6/scripts/strings/dec2bin.m

I was going to write a comment to the effect of "I won't have time to work on this before ~1 month", but I got nerd-sniped (I really should have worked on something else instead).

Anyway, I'm not too proud of how I implemented compatibility with arrays (and growing strings to the same lenghts), but I think the file I'm attaching is feature-complete.

If you want to refactor, the gist is:

% Perform the base conversion on:
i = uint64(-1*(int64(i)+1));
out = dec2base(i, 2, len)
% Then invert the output:
out = char((out=='0') + '0');

Things I'm not sure of:

  • How Matlab handles length extension. I implemented what made more sense to me. See comments in tests.
  • Apparently this was supposed not to work on floating point values? From my testing it does work on them, and I see no reason not to (I added a check to error after fmaxint(i)).
  • If there is a better way to concatenate values with the correct sign extension

Ideally, I'd have wanted (u)int<n> to default to <n> output digits, especially if <n> isn't default. But I can live with the attached implementation.

(file #53739)

Mayeul C. <mayeulc>
Wed 21 Sep 2022 03:28:02 PM UTC, comment #1: 

confirmed behavior on linux in 8.0.0 and windows, and that matlab behavior is comment #0 states is expected.

matlab 2022a:

>> dec2bin(int16(-4200))
ans =
    '1110111110011000'

>> dec2bin(int32(-4200))
ans =
    '1110111110011000'

>> dec2bin(int32(-4200),32)
ans =
    '11111111111111111110111110011000'

>> dec2bin(int32(-42000))
ans =
    '11111111111111110101101111110000'

>> dec2bin(-42000)
ans =
    '11111111111111110101101111110000'

Nicholas Jankowski <nrjank>
Project Member
Wed 21 Sep 2022 02:51:57 PM UTC, original submission:  

Consider:

> dec2bin(int16(-4200))
ans = 111111111111111
> dec2bin(int32(-4200))
ans = 1110111110011000

-4200 obviously fits in a 2-complemented signed 16 bit integer, this is a bug. The first line should return the same result as the second one.

While I am at it:

Octave:1> dec2bin(int32(-4200), 32)
ans = 00000000000000001110111110011000
Octave:2> dec2bin(int32(-42000))
ans = 1111111111111111111111111111111
Octave:3> dec2bin(-42000)
ans = 11111111111111110101101111110000

Octave:1 -- The leading zeroes should be leading ones for negative integers in 2-complement.

Octave:2 -- This is doubly wrong: wrong number of digits, and wrong result. The next line shows a seemingly-correct result

There seems to be some of the same behaviours with i64, but I'm soon hitting "dec2bin: negative inputs cannot be less than -flintmax () / 2".

Mayeul C. <mayeulc>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #53751:  dec2hex.patch added by arungiridhar (3KiB - text/x-patch)
file #53748:  dec2bin.patch_v2 added by arungiridhar (5KiB - application/octet-stream)
file #53747:  dec2bin.patch added by arungiridhar (2KiB - text/x-patch)
file #53739:  dec2bin.m added by mayeulc (8KiB - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by arb (Posted a comment)
  • -email is unavailable- added by arungiridhar (Updated the item)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by mayeulc (Submitted the item)
  •  

    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 project members can vote.

     

     

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-09-25 arungiridhar StatusConfirmed Ready For Test
    2022-09-24 arungiridhar Attached File- Added dec2hex.patch, #53751
    2022-09-24 arungiridhar Attached File- Added dec2bin.patch_v2, #53748
    2022-09-24 arungiridhar Attached File- Added dec2bin.patch, #53747
    2022-09-21 mayeulc Attached File- Added dec2bin.m, #53739
    2022-09-21 nrjank StatusNone Confirmed
        Release7.2.0 dev
        Operating SystemGNU/Linux Any

    Back to the top


    Powered by Savane 3.9