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
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: ↓ 5 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: ↓ 6 ↓ 7 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
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?
comment:8 follow-up: ↓ 9 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.
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.
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.