Opened 7 years ago

Closed 7 years ago

#4939 closed defect (fixed)

mod(x,y) with x < 0 computed incorrectly in algorithms

Reported by: Francesco Casella Owned by: Martin Sjölund
Priority: high Milestone: 1.13.0
Component: Code Generation Version:
Keywords: Cc: Martin Sjölund

Description

Please run this simple test case (also attached)

model TestMod
  Integer j, j_mod;
  Integer k, k_mod;
algorithm
  when initial() then
    j := -5;
    j_mod := mod(j,3);
  end when;
equation
    k = -5;
    k_mod = mod(k,3);
end TestMod;

k_mod is correcly computed in the equation section as 1, while j_mod is incorrectly computed in the algorithm section as -2. This is a bit weird, as the same Modelica function is used in both cases.

@lochel, could you have a quick look if this is an easy fix?

Attachments (1)

TestMod.mo (203 bytes ) - added by Francesco Casella 7 years ago.

Download all attachments as: .zip

Change History (11)

by Francesco Casella, 7 years ago

Attachment: TestMod.mo added

comment:1 by Francesco Casella, 7 years ago

Summary: mod() with negative argument works incorrectly in algorithmsmod(x,y) with x < 0 computed incorrectly in algorithms

comment:2 by Martin Sjölund, 7 years ago

This is a problem with the generated code for integer mod(). It is missing a floor() call. It is slightly problematic that Ceval and Simplify do not simply call mod(), and instead have their own definition without this problem so I will fix the generated code and these modules.

comment:3 by Lennart Ochel, 7 years ago

The difference is when the mod function is evaluated. It works correctly if it is evaluated during compile time. The runtime implementation is wrong. I fixed it in the generated c code but now I have to check where this code is actually generated.

Last edited 7 years ago by Lennart Ochel (previous) (diff)

comment:4 by Lennart Ochel, 7 years ago

It seems Martin was faster...

comment:6 by Martin Sjölund, 7 years ago

The bootstrapped compiler depends on generating the wrong code for integer modulus, so it's not that easy to fix this.

comment:7 by Martin Sjölund, 7 years ago

So the specification says to use:

return x-floor(x/y)*y;

Which works fine for doubles. But for integers, this fails if the integer is so large that it does not fit in a double precision float.

The old code had:

return x%y;

Which returns negative values in some cases. I think the correct code would then be:

tmp = x%y;
return tmp < 0 ? tmp+y : tmp;

in reply to:  7 comment:8 by Lennart Ochel, 7 years ago

Replying to sjoelund.se:

Which returns negative values in some cases. I think the correct code would then be:

tmp = x%y;
return tmp < 0 ? tmp+y : tmp;

That should work just well.

comment:9 by Martin Sjölund, 7 years ago

Owner: changed from Lennart Ochel to Martin Sjölund
Status: newassigned

comment:10 by Martin Sjölund, 7 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.