Opened 4 years ago

Closed 3 years ago

#6173 closed defect (fixed)

Inconsistent flattening of for loops in NF with -d=nonfScalarize

Reported by: casella Owned by: perost
Priority: high Milestone: 1.17.0
Component: New Instantiation Version:
Keywords: Cc: massimo.fioravanti@…, federico.terraneo@…, adrpo, sjoelund.se

Description

Please consider the following test case

package TestNoScalarize
  model A
    B[100] b;
  equation
    for i in 1:100 loop
      b[i].c[10] = 2;
    end for;
  end A;

  model B
    Real[10] c;
  end B;
end TestNoScalarize;

The output of non-scalarized flattening of A is

class TestNoScalarize.A
  Real[100, 10] b.c;
equation
  for i in 1:100 loop
    b[i].c[10] = 2.0;
  end for;
end TestNoScalarize.A;

For consistency with the declarations, it should rather be

class TestNoScalarize.A
  Real[100, 10] b.c;
equation
  for i in 1:100 loop
    b.c[i,10] = 2.0;
  end for;
end TestNoScalarize.A;

@perost, would it be possible to fix this without too much effort?

Thanks!

Change History (14)

comment:1 Changed 4 years ago by perost

It would be quite trivial to move the subscripts to the end of each component reference, but I'm not sure what the consequences will be. For the frontend we can just do it at the end to avoid any issues, but the backend might run into problems if the subscripts are no longer in the correct places.

comment:2 Changed 4 years ago by adrpo

  • Summary changed from Inconsistent glattening of for loops in NF with -d=nonfScalarize to Inconsistent flattening of for loops in NF with -d=nonfScalarize

comment:3 Changed 4 years ago by Karim.Abdelhak

It actually is something different i think... right? This is pretending there is only one model b with a matrix c as an attribute. I am not really sure this works out in the long run... at least for records we need to make an exception here because we want to keep the structure intact.

comment:4 follow-up: Changed 4 years ago by casella

  • Cc adrpo sjoelund.se added

Replying to perost:

It would be quite trivial to move the subscripts to the end of each component reference, but I'm not sure what the consequences will be. For the frontend we can just do it at the end to avoid any issues, but the backend might run into problems if the subscripts are no longer in the correct places.

Maybe for the time being you could implement this feature under an experimental debug flag, which is normally deactivated? This would allow us to proceed with our work.

More in general, the question is "What is a non-scalarized flattened Modelica model"? The question is not trivial at all.

In fact, the question "What is a (scalarized) flattened Modelica model" is finding an answer within the working group on flat Modelica of the EMPHYSIS project (see summary). I understand this is almost finalized. @perost and/or @adrpo and/or @sjoelund.se are involved directly in the project and can comment on that. In that case, it was an explicit requirement that flat Modelica is (or almost is) a strict subset of Modelica, so you don't need a different syntax and parser. This effort already took some time.

Answering the original question is even harder and we sure can't do that in the next few days, but at least we need to start the discussion, and maybe agree on some subsets of all possible models, which are the ones we are currently tackling with our experimental compiler.

More specifically, we understood that, as long as we have (arrays of) models containing (arrays of) Reals, everything can be boiled down to multi-dimensional arrays of Reals, and to array equations handling them.

In the case of records, if you have a.b.c, I understand at most one among a, b, and c can be a record name R, so you can declare the flat model as R a.b.c[x, y, z]. For example, consider this test case

package TestRecordArray
  model A
    B[100] b;
  equation
    for i in 1:10 loop
      b[i].c[10] = Complex(i,0);
    end for;
  end A;

  model B
    Complex[10] c;
  end B;
end TestRecordArray;

Curently, if you flatten TestRecordArray.A with -d=nonfScalarize you get

class TestRecordArray.A
  Real[100, 10] b.c.re "Real part of complex number";
  Real[100, 10] b.c.im "Imaginary part of complex number";
equation
  for i in 1:10 loop
    b[i].c[10] = Complex.'constructor'.fromReal(/*Real*/(i), 0.0);
  end for;
end TestRecordArray.A;

I guess what you should get is instead

operator Record Complex
  Real re;
  Real im;
  ...
end Complex;

class TestRecordArray.A
  Complex[100, 10] b.c;
equation
  for i in 1:10 loop
    b.c[i,10] = Complex.'constructor'.fromReal(/*Real*/(i), 0.0);
  end for;
end TestRecordArray.A;

Then it would be up to the backend to figure out if and how to expand the records to their Real components, or keep them together. We already know how to deal with this configuration in our compiler, so we can support Complex numbers, which are useful for demonstrate big power grid models.

Of course you could have records containing records, but I'm not sure how many libraries around actually need that (I'm aware of none).

comment:5 in reply to: ↑ 4 ; follow-ups: Changed 4 years ago by perost

Replying to casella:

Maybe for the time being you could implement this feature under an experimental debug flag, which is normally deactivated? This would allow us to proceed with our work.

Sure, I can implement that tomorrow.

In the case of records, if you have a.b.c, I understand at most one among a, b, and c can be a record name R, so you can declare the flat model as R a.b.c[x, y, z].

You can as you mention have records that contain records so any of a, b, and c can be a record in this case.

comment:6 in reply to: ↑ 5 Changed 4 years ago by casella

Replying to perost:

Replying to casella:

Maybe for the time being you could implement this feature under an experimental debug flag, which is normally deactivated? This would allow us to proceed with our work.

Sure, I can implement that tomorrow.

That would be great, thanks!

comment:7 in reply to: ↑ 5 Changed 4 years ago by casella

Replying to perost:

Replying to casella:

In the case of records, if you have a.b.c, I understand at most one among a, b, and c can be a record name R, so you can declare the flat model as R a.b.c[x, y, z].

You can as you mention have records that contain records so any of a, b, and c can be a record in this case.

Sure.

The question is, do we really need to support records of records and non-scalarized equations at the same time? I'm not aware of any library that uses that combination. except, perhaps, Buildings - I'm not 100% sure about that.

In that case, we should declare the variables using the enclosing record types, and then use whatever is necessary in the equation section, depending on whether the records are accessed as a whole, or by individual elements. For example, consider this test package

package Test
  record R1
    Real x;
    Real y;
  end R1;

  record R2
    R1 ra;
    R1 rb;
  end R2;

  model A
    B[100] b;
  equation
    for i in 1:100 loop
      b[i].c[10] = R2(R1(1, 0), R1(i,1));
      b[i].c[1].rb = R1(i,0);
    end for;
  end A;

  model B
    R2 c[10];
  end B;
end Test;

Model Test.A should be flattened into

model A
  R2 b.c[100,10];
equation
    for i in 1:100 loop
      b.c[i,10] = R2(R1(1, 0), R1(i,1));
      b.c.rb[i,1] = R1(i,0);
    end for;
end A;

What do you think?

Last edited 3 years ago by casella (previous) (diff)

comment:8 follow-up: Changed 4 years ago by mahge930

Do not forget slices. If both a and b array types and b "has" an a, then b.a[i] and b[i].a can both exist and should be not flattened to the same thing.

Basically every subscript must exist in the final flattened cref if the original had at least one.

record A
  Real[4] a;
end A;

model B
  A[3] b;
equation
  // normal
  for i in 1:4 loop
    b[3].a[i] = 1;
    // b.a[3][i] = {1, 2, 3};
  end for;

  // sliced. 
  for i in 1:4 loop
    b.a[i] = {1, 2, 3};
    // { b[1].a[i], b[2].a[i], b[3].a[i] } = {1, 2, 3}
    // b.a[:][i] = {1, 2, 3};
  end for;

  for i in 1:3 loop
    b[i].a = {1, 2, 3, 4};
    // { b[i].a[1], b[i].a[2], b[i].a[3], b[i].a[3] } = {1, 2, 3, 4}
    // b.a[i][:] = {1, 2, 3};
  end for;
end B;

So the flattened crefs for last two loops should have the missing subs as whole dimension indices.

Last edited 4 years ago by mahge930 (previous) (diff)

comment:9 in reply to: ↑ 8 Changed 4 years ago by casella

Replying to mahge930:

So the flattened crefs for last two loops should have the missing subs as whole dimension indices.

Absolutely! Thanks @mahge930 for pointing this out.

comment:10 Changed 4 years ago by perost

The proposed behaviour is now implemented in 1d285900/OpenModelica, it's enabled by the debug flag combineSubscripts.

comment:11 Changed 4 years ago by casella

Thanks @perost, we'll try this out and report ASAP

comment:12 Changed 4 years ago by casella

  • Milestone changed from 1.17.0 to 1.18.0

Retargeted to 1.18.0 because of 1.17.0 timed release.

comment:13 Changed 3 years ago by casella

  • Milestone 1.18.0 deleted

Ticket retargeted after milestone closed

comment:14 Changed 3 years ago by casella

  • Milestone set to 1.17.0
  • Resolution set to fixed
  • Status changed from new to closed

TestNoScalarize.A is now flattened as expected if -d=combineSubscripts is set.

The problem remains open for Test.A. We can continue that discussion in #8131 and close this ticket.

Note: See TracTickets for help on using tickets.