Skip to content
This repository has been archived by the owner on May 18, 2019. It is now read-only.

Add support for inline of some if-statements #2076

Closed
wants to merge 5 commits into from

Conversation

sjoelund
Copy link
Member

If-statements that look like if-expressions can now be inlined.
This fixes an issue reported in ticket:4423.

@OpenModelica-Hudson
Copy link
Member

The test suite is unstable according to OpenModelica_TEST_PULL_REQUEST 2017-12-13_15-54-40.

@OpenModelica-Hudson
Copy link
Member

The test suite is unstable according to OpenModelica_TEST_PULL_REQUEST 2017-12-15_08-58-39.

@sjoelund
Copy link
Member Author

@lochel @vruge @casella - with my changes somehow solveSimpleEquations (initialization) destroys one equation: it sets f(...) = volume1.medium.T to 1.0 = volume1.medium.T and doesn't add the operation in the debugger. The 1.0 expression is ... wrong.

@sjoelund
Copy link
Member Author

It seems that solveSimpleEquationsWork returns solved=false, but modifies the equation anyway... Perhaps I can fix this myself next week:

solved simple equation volume1.medium.h:VARIABLE
volume1.medium.T = Modelica.Media.Examples.Tests.Components.PortVolume$volume1.Medium.derivsOf_ph(volume1.medium.p, volume1.medium.h, Modelica.Media.Examples.Tests.Components.PortVolume$volume1.Medium.getPhase_ph(volume1.medium.p, volume1.medium.h)).T
=>
-1.0 = -volume1.medium.T

Which seems really wrong since h should be on the lhs.

@vruge
Copy link
Member

vruge commented Dec 16, 2017

@sjoelund
From the idea solveSimpleEquationsWork don't destroy the equation if return solved=false.
If solveSimpleEquationsWork can destroy equation for case solved=false then case solved=true can, too

Maybe, in
https://github.com/OpenModelica/OMCompiler/blob/master/Compiler/BackEnd/ExpressionSolve.mo#L1241 InlineExp has some issues.

@sjoelund
Copy link
Member Author

@vruge - as far as I can see no call is inlined (at least inlineCall doesn't do anything here). #2083 I tried to only do ExpressionSolve if solved=true was given; it seems to make one model succeed (same kind of error as the one I found before), and changes the results of other models. I think perhaps I should try to fix whatever code returns bad expressions instead.

@vruge
Copy link
Member

vruge commented Dec 18, 2017

Maybe, I would check
https://github.com/OpenModelica/OMCompiler/blob/master/Compiler/BackEnd/ExpressionSolve.mo#L1717

because inlining has impact of this function

@sjoelund
Copy link
Member Author

It seems to be:

outLhs := expAddX(e, outLhs, inExp3);

Which is e=f(x, volume1.medium.h, g(y, volume1.medium.h)).T, outLhs=0.0, inExp3=volume1.medium.h which gives 1.0 and the result.

@sjoelund
Copy link
Member Author

It seems Expression.expandFactors is called and it sends a default that the empty list should be returned for unknown expressions; this seems quite wrong and is why it will replace the expression with {}.

@OpenModelica-Hudson
Copy link
Member

The job failed to compile; for details, see OpenModelica_TEST_PULL_REQUEST 2017-12-19_06-33-28.

@OpenModelica-Hudson
Copy link
Member

The test suite is unstable according to OpenModelica_TEST_PULL_REQUEST 2017-12-19_08-24-55.

@OpenModelica-Hudson
Copy link
Member

The test suite is unstable according to OpenModelica_TEST_PULL_REQUEST 2017-12-19_08-59-19.

@sjoelund
Copy link
Member Author

@casella
The remaining models failing in the testsuite are two ThermoPower models, for example ThermoPower.Test.DistributedParameterComponents.TestFlow1D2phDB_hf.mos:

assert | error   | IF97 medium function tsat called with too low pressure
| | |       | p = 0 Pa <= 611.657 Pa (triple point pressure)

And a buildings model giving a diff I'm a little bit scared of.

-vol.dynBal.medium.MM = 1.0;
+vol.dynBal.medium.MM = 1.0 / (vol.dynBal.medium.Xi[1] / <EMPTY(scope: Buildings.Fluid.Interfaces.ConservationEquation$vol$dynBal.Medium.BaseProperties$vol$dynBal$medium, name: steam.MM, ty: Real(String quantity PARAM DAE.EQBOUND("MolarMass", SOME("MolarMass"), C_PARAM, [DEFAULT VALUE]), String unit PARAM DAE.EQBOUND("kg/mol", SOME("kg/mol"), C_PARAM, [DEFAULT VALUE]), Real min PARAM DAE.EQBOUND(0.0, SOME(0.0), C_PARAM, [DEFAULT VALUE])))> + (1.0 - vol.dynBal.medium.Xi[1]) / <EMPTY(scope: Buildings.Fluid.Interfaces.ConservationEquation$vol$dynBal.Medium.BaseProperties$vol$dynBal$medium, name: dryair.MM, ty: Real(String quantity PARAM DAE.EQBOUND("MolarMass", SOME("MolarMass"), C_PARAM, [DEFAULT VALUE]), String unit PARAM DAE.EQBOUND("kg/mol", SOME("kg/mol"), C_PARAM, [DEFAULT VALUE]), Real min PARAM DAE.EQBOUND(0.0, SOME(0.0), C_PARAM, [DEFAULT VALUE])))>);

@sjoelund
Copy link
Member Author

Which might be due to:

data->localData[0]->realVars[122] /* hex.sat.psat variable */

Which is supposed to be an alias of:

data->localData[0]->realVars[135] /* $cse4.psat variable */

And not a reference to:

data->localData[0]->realVars[122] /* $cse3.cp variable */

@casella
Copy link

casella commented Dec 19, 2017

p = 0 is quite bad, but I'm not sure if this used to work before

@sjoelund
Copy link
Member Author

It seems to just be bad code generation for some alias of record components which get messed up due to some difference between simulation and initialization.

If-statements that look like if-expressions can now be inlined.
This fixes an issue reported in ticket:4423.
Use DAE.RSUB() to handle general expressions. When doing inline of a
function call, we can use DAE.RSUB instead of failing; previously we
only handled component references passed to the call. Also added code
generation and simplifications for DAE.RSUB since it was only used
for MetaModelica previously.

This fixes some of the issues raised in ticket:4423.
@OpenModelica-Hudson
Copy link
Member

The test suite is unstable according to OpenModelica_TEST_PULL_REQUEST 2018-01-02_13-07-48.

The code generator gave the wrong variable indexes for aliases that
were not replaced in the backend. This prints some warnings or errors
for these cases so we can either fix the code (or replace the code
generator to handle aliases better).
@OpenModelica-Hudson
Copy link
Member

The test suite is unstable according to OpenModelica_TEST_PULL_REQUEST 2018-01-02_14-16-07.

@sjoelund
Copy link
Member Author

sjoelund commented Jan 2, 2018

ThermoPower.Test.DistributedParameterComponents.TestFlow1D2phChen.mos is now the only failing test. It fails in a torn NLS with 193 unknowns and 9 iteration variables (previously it was 195/9, but it seems something in there is now slightly different and ThermoPower.Water.f_chen keeps getting negative roots). Not sure if I need to fix it or not.

@lochel
Copy link
Member

lochel commented Jan 2, 2018

I guess this is due to the heuristics and not really an issue with your changes.

@sjoelund
Copy link
Member Author

sjoelund commented Jan 3, 2018

Yes, I realize now that the master fails during initialization for me as well (my previous comment was regarding that system that failed). Hudson fails during integration at time=116. So the model is probably hard to solve and doesn't quite work at the moment anyway. I guess I can try it on asap and see if tweaking the numerical tolerance makes Hudson happy.

@sjoelund
Copy link
Member Author

sjoelund commented Jan 3, 2018

The IDA solver said it was a (torn) linear system that was problematic. Disabling tearing on it didn't help. I'll just disable the simulation success part of the test :(

@OpenModelica-Hudson
Copy link
Member

The test suite is unstable according to OpenModelica_TEST_PULL_REQUEST 2018-01-03_08-37-08.

@OpenModelica-Hudson
Copy link
Member

The tests run correctly according to OpenModelica_TEST_PULL_REQUEST 2018-01-03_09-01-41.

OpenModelica-Hudson pushed a commit that referenced this pull request Jan 3, 2018
If-statements that look like if-expressions can now be inlined.
This fixes an issue reported in ticket:4423.

Belonging to [master]:
  - #2076
  - OpenModelica/OpenModelica-testsuite#810
OpenModelica-Hudson pushed a commit that referenced this pull request Jan 3, 2018
Use DAE.RSUB() to handle general expressions. When doing inline of a
function call, we can use DAE.RSUB instead of failing; previously we
only handled component references passed to the call. Also added code
generation and simplifications for DAE.RSUB since it was only used
for MetaModelica previously.

This fixes some of the issues raised in ticket:4423.

Belonging to [master]:
  - #2076
  - OpenModelica/OpenModelica-testsuite#810
OpenModelica-Hudson pushed a commit that referenced this pull request Jan 3, 2018
OpenModelica-Hudson pushed a commit that referenced this pull request Jan 3, 2018
The code generator gave the wrong variable indexes for aliases that
were not replaced in the backend. This prints some warnings or errors
for these cases so we can either fix the code (or replace the code
generator to handle aliases better).

Belonging to [master]:
  - #2076
  - OpenModelica/OpenModelica-testsuite#810
OpenModelica-Hudson pushed a commit to OpenModelica/OpenModelica-testsuite that referenced this pull request Jan 3, 2018
@sjoelund sjoelund deleted the ticket4423 branch January 3, 2018 09:09
@sjoelund
Copy link
Member Author

sjoelund commented Jan 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants