Opened 8 years ago
Closed 8 years ago
#4093 closed defect (fixed)
Many nightly tests fail since October 21
Reported by: | Rüdiger Franke | Owned by: | Lennart Ochel |
---|---|---|---|
Priority: | blocker | Milestone: | 1.11.0 |
Component: | Backend | Version: | |
Keywords: | Cc: | Patrick Täuber, Volker Waurich, Vitalij Ruge, Niklas Worschech |
Description
Have a look at:
Since October 21 the validation of many MSL and MSL_test examples fails with the Cpp runtime.
Moreover: the translation fails for C and Cpp for
PowerSystems.Examples.Spot.GenerationAC3ph.*Generator*
examples.
Change History (19)
comment:1 by , 8 years ago
Cc: | added |
---|
comment:2 by , 8 years ago
Rüdiger, can you have a look at the cpp runtime issue? Patrick and I will have a look at the compilation issues for the PowerSystems models.
follow-up: 10 comment:3 by , 8 years ago
Cc: | added |
---|
Well, I picked the NoiseExamples
that have a lot of new validation errors. It turns out that the situation got worse with 1692f27 (BackendDAE.ASSIGN can hold tuples on lhs).
It is good if duplicate function calls are avoided. It is bad the the generated Cpp code does not call the function anymore at all. This needs to be fixed first because it hides the effects from October 21.
See the following example:
model DIDtuple function update input Real pre_y1; input Real pre_y2; input Real u; input Real samplePeriod; output Real y1; output Real y2; algorithm y1 := pre_y1 + u * samplePeriod; y2 := pre_y2 + pre_y1 * samplePeriod + 0.5 * u * samplePeriod^2; end update; input Real u(start = -2); Real y1(start = 1), y2; parameter Real samplePeriod = 0.1; equation when sample(0, samplePeriod) then (y1, y2) = update(pre(y1), pre(y2), u, samplePeriod); end when; annotation(experiment(StopTime=1)); end DIDtuple;
comment:4 by , 8 years ago
Just keep in mind that we have also the backend module wrapFunctionCalls
to avoid duplicated function calls. Unfortunately, this Is not enabled by default yet.
comment:5 by , 8 years ago
I just started to debug the model GenerationAC3ph.TurbineGenerator
which fails while generating the SimCode structure. The issue is, that an assignment for a final parameter is to be generated, which has neither a binding nor an explicitly declared start value.
generator.c.L_s[1]:PARAM(unit = "H" protected = true final = true ) "L matrix stator dq0" type: Real [3]
comment:6 by , 8 years ago
Thanks rfranke for monitoring the testsuite and the issue with my commit. I will fix this.
@ lochel: I dont think that wrapFunctionCalls can avoid duplicated calls in case of an expanded tuple assignment.
(extObjVar, realVar) := someFunctionCall(someInputs);
which gets expanded to:
extObjVar := someFunctionCall(someInputs);
and
realVar:= someFunctionCall(someInputs);
follow-up: 8 comment:7 by , 8 years ago
I guess I inderduce some bugs. By the try to rewritte the model simplifyComplexFuntion.
Can someone(please) revert it around the day? Or I can do it on weekend.
@lochel Can you add please some examples from the pnlib, that we can catch the bug much earlier.
comment:8 by , 8 years ago
Replying to vitalij:
I guess I inderduce some bugs. By the try to rewritte the model simplifyComplexFuntion.
Can someone(please) revert it around the day? Or I can do it on weekend.
@lochel Can you add please some examples from the pnlib, that we can catch the bug much earlier.
This is the wrong ticket, see OMCompiler#1152. Anyway, I have opened OMCompiler#1165 to revert the changes.
comment:9 by , 8 years ago
I have fixed the generation of parameter equations with OMCompiler#1163.
comment:10 by , 8 years ago
Replying to rfranke:
Well, I picked the
NoiseExamples
that have a lot of new validation errors. It turns out that the situation got worse with 1692f27 (BackendDAE.ASSIGN can hold tuples on lhs).
It is good if duplicate function calls are avoided. It is bad the the generated Cpp code does not call the function anymore at all. This needs to be fixed first because it hides the effects from October 21.
I fixed the cpp codegen and added this model as a test. Are there any other particular models I can check regarding the cpp codegen?
comment:11 by , 8 years ago
@vwaurich: unfortunately the nightly tests did not improve -- obviously my example didn't cover all problems.
Instead of a comment // nothing to do
we get now a wrong call to DiscreteEvents::pre
with array argument. See e.g. Modelica.Blocks.Examples.NoiseExamples.Distributions
.
Commit 76cba8f attempts to correct this second issue ... I'd like to add the simulation of the Distributions example to the testsuite, but the reference results are missing ... so let's wait for the next nightly tests.
comment:12 by , 8 years ago
@rfranke: I see. Its in msl 3.2.2. Sorry, I havent found the test of the noise model because I was looking into ModelicaTest. I will care about it, if your commit doesnt do the job.
comment:13 by , 8 years ago
It became even worse tonight. To summarize:
- typeof(var) returns just CREF instead of T_ARRAY.
- SIMVAR's numArrayElement reports a dimension for a scalar time variable (
Modelica.Electrical.PowerConverters.Examples.ACDC.Rectifier1Pulse.Thyristor1Pulse_R
). - SIMVAR's arrayCref is NONE() for all but the first scalar array element (
PowerSystems.Examples.Spot.AC1ph_DC.Inverter
) - crefIsScalar returns false for a scalar array element
Let's see if 4a3540 and c60d86 will do it.
Is this thing called MineSweeper or OpenModelica?
follow-up: 18 comment:14 by , 8 years ago
It seems the cpp tests are back in place since: ea606a1b
This was actually a bug which did not take effect before. Nice to have it fixed.
Whats the status of the ticket now? Anything else that is broken?
comment:15 by , 8 years ago
The least remaining thing would be to fix the two operator() methods of RefArrayDim3 that take an index vector as well.
Moreover it should be checked if the row major ordering of RefArray introduces more bugs; e.g. during copy to value array or for value references. Alternatively make RefArray column major as well.
comment:16 by , 8 years ago
Cc: | added |
---|
comment:17 by , 8 years ago
From reading the code I get the impression that the intention was to have RefArray column major, like other Cpp arrays. For instance CodegenCpp.tpl calls SimCodeUtil.getVarIndexListByMapping with iColumnMajor=true
and a comment says "to conform to the cpp runtime array addressing".
PR 1193 makes a naive move into the intended direction: it changes all operator() of RefArray to column major. Moreover it removes listReverse
at two places in SimCodeUtil.mo -- no idea if this is correct; but the tests pass and world.y_label
of the DoublePendulum example contains the correct entries.
comment:18 by , 8 years ago
Replying to vwaurich:
This was actually a bug which did not take effect before. Nice to have it fixed.
Whats the status of the ticket now? Anything else that is broken?
I'm afraid the bug took effect because the code generation introduces too many RefArray now. Before October 21 the consecutive memory was detected correctly and simple StatArray were used instead.
Meaning that your fix and my pull request address a new issue that showed up, but not the root cause!
comment:19 by , 8 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Now the nightly tests run again AND there is no known bug with RefArray anymore. An attempt to test RefArray showed that the generated code is very inefficient, i.e. RefArray causes lot of headache during code generation, but is hardly exploited in the code. The follow-up ticket #4129 was opened.
This is related to the changes to the parameter handling. Probably, there is something wrong with the initialization of parameter in the cpp runtime.