The NF rejects supposedly legal code involving algorithms and discrete variables

[Francesco Casella]

In #5836, checks were introduced to make sure that discrete variables can only be matched to left-hand-sides of when-equations.

IN ​5aec587 additional sanity checks for discrete variables were introduced in the NF.

Consider the ​PowerGrids newInst report. After the recent fixes to the library, that previously contained incorrect discrete prefixes, many models are now running OK.

However, there are seven remaining failures, see e.g. ​this one, that have all the same root cause, which can all be traced to the code of the ​Bus Fault model:

  discrete Types.ComplexAdmittance Y(
    re(start = 0, fixed = true), 
    im(start = 0, fixed = true)) "Shunt admittance";
...
algorithm
   when pre(fault) then
     Y := 1/Complex(R, X);
   end when;

   when not pre(fault) then
     Y := Complex(0); 
   end when;

The Specification, ​Sect. 11.2.7 defines when-statements:

The algorithmic statements within a when-statement are activated when the scalar or any one of the elements of the vector-expression becomes true.

In this case, I understand the restrictions of ​Section 8.3.5.2 do not apply, because in this case we do not have when-equations, but rather an algorithm with when-statements.

Unfortunately, the NF aborts immediately after completing NFScalarize, complaining that

Error: Following variable is discrete, but does not appear on the
LHS of a when-statement: NTHV.Y.im.
Error: Following variable is discrete, but does not appear on the
LHS of a when-statement: NTHV.Y.re.

I'd say these messages are incorrect, because indeed NTHV.Y appears on the left-hand-side of all the assignments inside the algorithm, even though not directly, being it a complex record. This model (that runs fine in Dymola) is thus legal and should be accepted for further optimizations.

A couple of weeks ago the entire PowerGrids library worked flawlessly, so I guess this is due to some recent commit, that introduced a regression. It would be good if we can fix that in time for the final 1.16.0 release.

@perost, @Karim, would you mind having a look?

[Karim Adbdelhak]

Thanks for the analysis, i will have a look!

[Karim Adbdelhak]

I worked with following minimal model:

record R
  Real a;
  Real b;
end R;

model M
  discrete R r;
initial equation
  r = R(1,0);
equation
  when time > 0.5 then
    r = R(0,0);
  end when;
end M;

My old sanity check did not support records and reported:

[<interactive>:4:3-4:9:writable] Error: Following variable is discrete, but does not appear on the LHS of a when-statement: ‘r.b‘.
[<interactive>:3:3-3:9:writable] Error: Following variable is discrete, but does not appear on the LHS of a when-statement: ‘r.a‘.

I added support for records in ​PR6668 and it passes my check now. Unfortunately it fails during templates:

[CodegenC.tpl:5962:11-5962:11:writable] Error: Template error: No runtime support for this record assignment: r = R(0.0, 0.0).

But that is another error and i think other models will have the same problem using the new frontend.

The part my check broke is fixed again (if jenkins does not complain about my PR), but these models will still not compile. As far as i can see they did not pass template with the new frontend even before my sanity check, right?

[Karim Adbdelhak]

It is pushed, my check will no longer reject discrete records.

But i did not verify with the PowerGrids library, i will wait for the regression report!

I am a little worried because of above mentioned error during templates, it does not happen with the old frontend!

[Francesco Casella]

Replying to Karim.Abdelhak:

It is pushed, my check will no longer reject discrete records.

Thank Karim, this is great!

But i did not verify with the PowerGrids library, i will wait for the regression report!

Yep, we'll see.

I am a little worried because of above mentioned error during templates, it does not happen with the old frontend!

This sounds familiar to me. @perost, do you have any idea why this happens?

@mahge930, in case this cannot be fixed in the NF, could you have a look at it?

[Per Östlund]

Replying to casella:

This sounds familiar to me. @perost, do you have any idea why this happens?

It's because the OF splits the equation into

r.a = 0.0;
r.b = 0.0;

while the NF just keeps it as it is. The same thing happens with the OF if you have some kind of record expression on the rhs that can't be divided, like a function call that returns a record expression, so fixing it in the backend would be preferable.

I'm not sure how much this issue affects the libraries we support though, I don't remember seeing it before. If I've overlooked something and it's a common issue it would be fairly easy to have the NF mimic the OF behaviour.

[Karim Adbdelhak]

The new ​regression report contains more regressions regarding PowerGrids. But i cannot reproduce the error... i cloned PowerGrids from ​GitHub and tried repoducing the error for ​PowerGrids.Electrical.Test.OneBusImpedanceOneLoad

I used following .mos file in the cloned repository (+ using -d=newInst from command line):

loadFile("package.mo"); getErrorString();

buildModelFMU(PowerGrids.Electrical.Test.OneBusImpedanceOneLoad,fileNamePrefix="PowerGrids_PowerGrids_Electrical_Test_OneBusImpedanceOneLoad",fmuType="me",version="2.0",platforms={"static"}); getErrorString();

and it does not fail for me, just throws some warnings:

true
"Notification: Automatically loaded package Modelica 3.2.3 due to uses annotation.
Notification: Automatically loaded package Complex 3.2.3 due to uses annotation.
Notification: Automatically loaded package ModelicaServices 3.2.3 due to uses annotation.
"
"/home/kab/Repo/FH/promotion/modelicaLibs/PowerGrids/PowerGrids/Test/PowerGrids_PowerGrids_Electrical_Test_OneBusImpedanceOneLoad.fmu"
"Warning: The model contains alias variables with redundant start and/or conflicting nominal values. It is recommended to resolve the conflicts, because otherwise the system could be hard to solve. To print the conflicting alias sets and the chosen candidates please use -d=aliasConflicts.
Warning: The hideResult annotation could not be evaluated, probably due to missing annotation(Evaluate=true). It is set to 'isProtected' (=false) by default.
"

Am i missing something?

[Per Östlund]

Replying to Karim.Abdelhak:

Am i missing something?

The regression report is for master-fmi, i.e. not using the NF, and seems to be caused by changes in the PowerGrids library rather than the compiler. The regression testing for newInst doesn't seem to have run yet.

[Karim Adbdelhak]

Replying to perost:

Replying to Karim.Abdelhak:

Am i missing something?

The regression report is for master-fmi, i.e. not using the NF, and seems to be caused by changes in the PowerGrids library rather than the compiler. The regression testing for newInst doesn't seem to have run yet.

Ah i missed that, thanks!

[Francesco Casella]

Yes, the master-fmi job is badly broken, see e.g. #6048. We'll need to fix that once @sjoelund gets back from his holidays.

[Karim Adbdelhak]

It is fixed as shown in ​regression report.

The two regressions cannot possibly be the fault of my commit, that origins somewhere else. Some segmentation fault during simulation, they also show up in the other reports.

We can consider this ticket fixed, but may want to have a look at the failing two models.

[Francesco Casella]

Replying to Karim.Abdelhak:

We can consider this ticket fixed

Absolutely, thanks!

but may want to have a look at the failing two models.

Don't waste your time with that. Those models keep breaking and healing every other commit, you can check the previous report. No idea why. We need to push Martin to fix all that stuff.

[Mahder Alemseged Gebremedhin]

I am late to the party. Is the codegen issue fixed or remains?

[Karim Adbdelhak]

Replying to mahge930:

I am late to the party. Is the codegen issue fixed or remains?

Still not fixed.

[Francesco Casella]

@mahge390 if you want to address the code generation when the records are not split, that could help in many cases, though I do not have any test cases ready right now.