bugGNU Octave - Bugs: bug #53128, Warn about implicit casts from int...

 
 

bug #53128: Warn about implicit casts from int to double

Submitter:  Benjamin Buch <bebuch>
Submitted:  Mon 12 Feb 2018 10:02:58 AM UTC
   
 
Category:  Interpreter Severity:  1 - Wish
Priority:  3 - Low Item Group:  Feature Request
Status:  None Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 26 Feb 2019 07:17:46 PM UTC, comment #7: 

To attempt to summarize, this is about adding a warning message to subsasgn, i.e. the binary assignment operator '=' when RHS is a subscripted expression, when the left operand is a double precision type and the right operand is an integer type.

If there is no subscripted assignment, the left operand is simply overwritten and gets the type of the right operand, no warning should be issued.

This is not a Matlab compatibility issue, this would be an enhancement for users who are not aware of this well-defined typecasting behavior on assignment. If we do add this warning to Octave, it should probably be disabled by default, but interested users can enable it using the warning ID.

Mike Miller <mtmiller>
Group Member
Tue 13 Feb 2018 09:34:59 AM UTC, comment #6: 

I will try to write it a little more clearly and answer the questions.

> Wait, why would you write a doubly nested for loop to multiply a matrix by a scalar?


This is a simplified test case which should illustrate the problem. Real code would of course be more complex and could not be shortened.

> In any case, I believe the reason that Matlab (and, therefore, Octave) treats mixed double/int operations as int is because a constant like "2" is actually a double precision object (and it's highly unlikely that's going to change).


Correct.

> However, maybe we could have a warning for mixed double/int operations when the value of the double is not an exact integer. Is that what you are asking for?


No.

> Or do you really want a warning for any mixed int/double operations?


No.

> It would be really inconvenient for expressions like "some_int8_value * 2" to result in a double value instead of an int8 value.


I'm not proposing that. It would break code and compatibility with Matlab.


Here's what I propose: Warn about implicit casts from int to double.

I will try to illustrate it again with the example:


img = uint8(ones([600, 800]));
factor = 1.7;

img_size = size(img);
result = zeros(img_size); % type is double

for c = 1:img_size(2)
    for r = 1:img_size(1)
        result(r,c) = img(r,c) * factor;
                     %^ integer multiplication: okay
                   %^ implicit cast from int to double: warning!
    end
end

% result has type double an values 2


It is the implicit cast where I want to issue a warning, because in most cases it is a result of wrong codes. The error is very difficult to recognize, because in the working environment result is written as a matrix of type double and in a more complex example may also contain comma numbers.

The situation changes fundamentally if result is not defined:


img = uint8(ones([600, 800]));
factor = 1.7;

img_size = size(img);
% result = zeros(img_size); % result is implicitly defined in the loop

for c = 1:img_size(2)
    for r = 1:img_size(1)
        result(r,c) = img(r,c) * factor;
                     %^ integer multiplication: okay
                   %^ define, resize and assign result: okay
    end
end

% result has type unit8 an values 2


In the working environment result is listed as a uint8 matrix. No warning is necessary here, as the error is much easier to recognize.

Generalizing the case leads to the proposal I made: Warn about implicit casts from int to double.

More examples:


a = uint8(5);              % okay
b = zeros([1, 1])          % okay
b(1, 1) = uint8(5)         % warning
b = ones([1 1]) * uint8(5) % okay


Benjamin Buch <bebuch>
Tue 13 Feb 2018 08:06:48 AM UTC, comment #5: 

Generally, I'm also in favour of emitting warnings in situations that have some significant probability of arising due to a user being not familiar with every last clause of Octave/Matlab syntax. Whether this is the case here would need to be discussed.

I think that performance would not be an issue. Remember that on the machine code level, this kind of check obviously is done already now, as the processor has to know how it has to interpret the contents of the memory addresses you pass to it. So what would be needed is to check, once this branch is taken, whether this warning is set, and if yes, to emit it. And remember further that this has to be done only once per Octave-instruction. That is, in the double for-loop in the toy test case below, it will very likely be submerged by the effort  of the interpreter to parse the line once per iteration, while in a matrix/matrix or matrix/scalar operation it has to be checked only once, because each entry of such a numeric object has the same type.

Perhaps there would be a performance hit when you use arrayfun on an internal function, assuming arrayfun is efficient. No, I doubt it, consider


a=rand(1000,1000);
b=rand(1000,1000);
tic;c=a+b;toc
tic;c=arrayfun(@plus,a,b);toc


Michael Leitner <mleitner>
Mon 12 Feb 2018 07:35:45 PM UTC, comment #4: 

Might also be a big performance hit to add this kind of check and optional warning in every mixed type binary operation.

Mike Miller <mtmiller>
Group Member
Mon 12 Feb 2018 07:17:54 PM UTC, comment #3: 

Sorry, my statement may have been a little less clear than it could have been.

It would be really inconvenient for expressions like "some_int8_value * 2" to result in a double value instead of an int8 value.

John W. Eaton <jwe>
Group administrator
Mon 12 Feb 2018 07:16:01 PM UTC, comment #2: 

In any case, I believe the reason that Matlab (and, therefore, Octave) treats mixed double/int operations as int is because a constant like "2" is actually a double precision object (and it's highly unlikely that's going to change).  So, it would be really inconvenient to not be able to write "some_int8_value * 2" and have the result be double instead of int8.

However, maybe we could have a warning for mixed double/int operations when the value of the double is not an exact integer.  Is that what you are asking for?  Or do you really want a warning for any mixed int/double operations?

John W. Eaton <jwe>
Group administrator
Mon 12 Feb 2018 07:09:25 PM UTC, comment #1: 

Wait, why would you write a doubly nested for loop to multiply a matrix by a scalar?

John W. Eaton <jwe>
Group administrator
Mon 12 Feb 2018 10:02:58 AM UTC, original submission:  

I have a source similar to this:


img = uint8(ones([600, 800]));
factor = 1.7;

img_size = size(img);
result = zeros(img_size); % type is double

for c = 1:img_size(2)
for r = 1:img_size(1)
result(r,c) = img(r,c) * factor;
end
end


The result is, that all values in result are of type double and have value 2. This is because the multiplication result is of type uint8. So far so good, but within the assignment the result is implicit casted to double. If you don't expect the multiplication result is of type int, this is an bug, that is very very hard to find.

No variable indicates that an integer multiplication has taken place. Most of the Octave users around me are not aware that there are any integers in Octave at all! Those who have experience with other programming languages will not expect that a binary operation of int and double will have an int as the result.

While in most other programming languages the implicit conversion from double to int is a bug, in Octave it is the other way round. Most of the implicit conversions from int to double take place with faulty code. Please issue a warning if an implicit conversion from int to double takes place.

I have colleagues who have spent hours and sometimes several days and with the help of other colleagues are desperate for such mistakes.

(Same request to Matlab with Case Number 02956888)

Benjamin Buch <bebuch>

 

(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 mleitner (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by bebuch (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 group members can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-02-26 mtmiller Carbon-CopyRemoved 80942 -
    2019-02-26 mtmiller Severity3 - Normal 1 - Wish
        Priority5 - Normal 3 - Low
        Release4.0.0 dev

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code