Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#3885 closed defect (fixed)

inStream and Evaluate Parameter

Reported by: vitalij Owned by: vitalij
Priority: blocker Milestone: 1.13.0
Component: Backend Version:
Keywords: Cc: rfranke, casella, wbraun, vwaurich, perost

Description (last modified by vitalij)

we should transform inStream() after evaluate parameter and removeSimpleEquation

e.g. in Modelica.Fluid.Examples.Explanatory.MomentumBalanceFittings
we find

rightAdaptor.m_flow:VARIABLE(min = if rightAdaptor.allowFlowReversal then -9.999999999999999e+59 else 0.0 max = 100000.0 start = rightAdaptor.m_flow_start unit = "kg/s" )  "Mass flow rate in design flow direction" type: Real

so we need evaluate rightAdaptor.allowFlowReversal and then expand inStream()

see #3430 for more detials.

Attachments (3)

TestStreamConnectors.mo (26.4 KB) - added by casella 6 years ago.
TestStreamConnectorsNoActualStream.mo (24.6 KB) - added by casella 6 years ago.
TestStreamConnectorsNoActualStreamEvaluateParams.mo (27.7 KB) - added by casella 6 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 8 years ago by vitalij

  • Description modified (diff)
  • Summary changed from inStream and Eval Parameter to inStream and Evaluate Parameter

comment:2 Changed 8 years ago by adrpo

  • Cc perost added

Added perost to CC as he implemented inStream/actualStream operators.

comment:3 Changed 8 years ago by perost

That would be possible I guess, but it would mean that we'd have to keep all the connection information until the backend. But I don't see what the issue is, why can't you evaluate rightAdaptor.allowFlowReversal as it is?

comment:4 Changed 8 years ago by vitalij

How I understand inStream() reduce his neighborhood depend on min/max. So if we transform inStream() in the sum before merge alias or evaluate constant/parameter
e.g. min =if rightAdaptor.allowFlowReversal then -9.... the reducation of the neighborhood failed.

comment:5 follow-up: Changed 8 years ago by perost

I'm sorry, I don't know what you mean by neighbourhood.

comment:6 in reply to: ↑ 5 Changed 8 years ago by vitalij

Replying to perost:

I'm sorry, I don't know what you mean by neighbourhood.

Perhaps, I'm wrong. I mean something like the fixes from Rüdiger
see
treat_reductions_to_empty_connection sets special isZeroFlow-check.

comment:7 Changed 8 years ago by vitalij

There some notes allowFlowReversal #3430.
I'm using +d=dumpdaelow and find in the dump min =if rightAdaptor.allowFlowReversal then -9....
Additional we have min/max values just after removeSimpleEquation.

In the dump we don't have anymore inStream() so we should transnform inStream() in something like inStreamBackend()?!

Last edited 8 years ago by vitalij (previous) (diff)

comment:8 follow-up: Changed 8 years ago by perost

Ok, I see. We use min/max to simplify inStream, but those might not be evaluated until the backend. So the real issue is really the whole structural parameter confusion.

I don't really like the idea of moving the evaluation of inStream to the backend, for memory and efficiency reasons. But I'm no expert on stream connectors, I just implemented them according to the spec. Maybe Rüdiger or someone else has some input on this. Or maybe we should discuss it on the meeting tomorrow.

comment:9 Changed 8 years ago by vitalij

I hope we don't need evlauateinStream(). e.g. inStream() transform in
inStreamBackend(list1, list2, eps) which we can link agian an inline C/C++
inStreamCode(array1, array2, n, m, eps) (like semiLinear)
function.

So we need to add new derivation rule and reduce lis1 and list2 after removeSimpleEquation...

Maybe Rüdiger or someone else has some input on this. Or maybe we should discuss it on the meeting tomorrow

good idea :)

Last edited 8 years ago by vitalij (previous) (diff)

comment:10 in reply to: ↑ 8 Changed 7 years ago by casella

  • Component changed from *unknown* to Frontend
  • Milestone changed from Future to 2.0.0
  • Owner changed from somebody to perost
  • Status changed from new to assigned

Replying to perost:

Ok, I see. We use min/max to simplify inStream, but those might not be evaluated until the backend. So the real issue is really the whole structural parameter confusion.

I don't really like the idea of moving the evaluation of inStream to the backend, for memory and efficiency reasons. But I'm no expert on stream connectors, I just implemented them according to the spec. Maybe Rüdiger or someone else has some input on this. Or maybe we should discuss it on the meeting tomorrow.

Here is the required input.

In general, flow reversal can occur, so for connection among more than two stream connectors, inStream() results in a fairly elaborate expression, including conditional expressions. If, however, it can be determined a priori a that a fluid will never flow out of a certain connector (i.e., its flow variable has min = 0), then the corresponding term can be removed for good from the inStream() expression. This can lead to very drastic simplifications in the model structure.

Now, in many libraries (including Modelica.Fluid and ThermoPower), the choice whether or not flow reversal can happen at a certain connector is not fixed, but it depends on some parameter (e.g., Boolean allowFlowReversal. The typical pattern is

FluidPort port(m_flow(min = 
  if allowFlowReversal then Modelica.Constant.inf else 0.0));

From this point of view, all parameters that determine the min attribute of flow variables of stream connectors are structural parameters, and they should be evaluated as early as possible, before BLT analysis and code generation. On the other hand, note that these min attributes can only influence the actual expressions of inStream() operators and their dependencies on other variables.

@perost, can you deal with this as with all other structural parameters, i.e., make sure that those min attributes are evaluated by the front end before computing the inStream() expressions?

comment:11 Changed 7 years ago by vitalij

Note: some motivation to do simplification for inStream in Backend was:

1) many paramter allowFlowReversal has(ot user can help with) an annotion(Evaluate=True)
2) Only after removeSimpleEquation we can find min,max value for some variable and finally simplify const streams like in #4441 to zero and not posetiveMax(.) = max(0,eps) = eps

comment:12 follow-up: Changed 7 years ago by vitalij

Proposal:

  • inStream interduce -> posetiveMax(cref, eps) and posetiveMax(-cref, eps)
  • simpliy posetiveMax in Backend after removeSimpliEquation and EvaluateParameter and before IndexREduction
    • posetiveMax(cref, eps) = cref where variable is constant >= 0
    • posetiveMax(-cref, eps) = cref where variable is constant <= 0
    • posetiveMax(cref, eps) = 0 if variable(cref).min >= 0
    • posetiveMax(-cref, eps) = 0 if variable(cref).max <= 0

comment:13 Changed 6 years ago by vitalij

  • Resolution set to fixed
  • Status changed from assigned to closed

fixed with PR2319.

comment:14 Changed 6 years ago by wbraun

  • Milestone changed from 2.0.0 to 1.13.0

comment:15 in reply to: ↑ 12 Changed 6 years ago by casella

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to vitalij:

Proposal:

  • inStream introduce -> positiveMax(cref, eps) and positiveMax(-cref, eps)
  • simpliy positiveMax in Backend after removeSimpliEquation and EvaluateParameter and before IndexREduction
    • positiveMax(cref, eps) = cref where variable is constant >= 0
    • positiveMax(-cref, eps) = cref where variable is constant <= 0
    • positiveMax(cref, eps) = 0 if variable(cref).min >= 0
    • positiveMax(-cref, eps) = 0 if variable(cref).max <= 0

There are some errors here, from what I understand it should be

  • positiveMax(-cref, eps) = -cref where variable is constant <= 0
  • positiveMax(cref, eps) = 0 if variable(cref).max <= 0
  • positiveMax(-cref, eps) = 0 if variable(cref).min >= 0

comment:16 Changed 6 years ago by casella

  • Component changed from Frontend to Backend
  • Owner changed from perost to vitalij
  • Priority changed from critical to blocker
  • Status changed from reopened to assigned

comment:17 Changed 6 years ago by casella

I actually got a wrong, non-physical behaviour in a customer model that I cannot post here for confidentiality reasons. Detailed analysis of the -d=optdaedump output turned out to be consistent with the implementation proposed in comment:12, which I think is not correct.

I guess #PR2319 should be amended to reflect the definitions in comment:15.

comment:18 Changed 6 years ago by vitalij

  • Status changed from assigned to accepted

comment:19 Changed 6 years ago by casella

Regarding the topic of min/max merging of alias variables involved in the inStream computation, see #5099

comment:20 Changed 6 years ago by vitalij

maybe fixed with PR2622 and PR2623

comment:21 follow-up: Changed 6 years ago by casella

I prepared some tests for stream connectors that could be added to the testsuite, see the attached package. I ran them all with -d=optdaedump and analyzed how some stream variables are computed. I reported the analysis in the documentation layer of the test cases. The good news is that the optdaedump output is always correct and consistent with my analysis. The bad news is that in all these cases the front-end actually manages the simplification, so that the equations passed to the back-end are actually not amenable to further manipulation by simplifyInStream, so these test cases are of no use to check the correct behaviour of simplifyInStream.

This is consistent with the fact that when I tried to reproduce the bug in our customer models by removing unnecessary details, we got the correct results. Apparently, only in a handful of more complex cases (which is hard to reproduce) the front-end fails to introduce the appropriate simplifications for some reason, and only then the simplifyInStream function of the back-end kicks in.

@perost can you shed some light as to when the front-end (both old and new) can manage the simplifications in inStream due to min>=0 flow variables and when it cannot?

comment:22 Changed 6 years ago by casella

  • Resolution set to fixed
  • Status changed from accepted to closed

In the meantime, I checked our customer's model with the latest nightly after @vitalij's commits, and the results are now correct. Thanks Vitalij!

It would be good if we could have some more test cases for simplifyInStream, but that's beyond the scope of this ticket.

Changed 6 years ago by casella

comment:23 in reply to: ↑ 21 ; follow-up: Changed 6 years ago by perost

Replying to casella:

@perost can you shed some light as to when the front-end (both old and new) can manage the simplifications in inStream due to min>=0 flow variables and when it cannot?

I don't really understand the question, but both frontends remove stream connectors with zero flow before generating inStream equations (which for outside connectors means max == 0 and for inside connectors min == 0). They don't do any constant evaluation, so they rely on the min/max attributes having been evaluated in an earlier phase. The new frontend evaluates those attributes if they have constant bindings (i.e. is constant or structural parameter), the old frontend I think tries its best to always evaluate them.

comment:24 in reply to: ↑ 23 Changed 6 years ago by casella

Replying to perost:

Replying to casella:

@perost can you shed some light as to when the front-end (both old and new) can manage the simplifications in inStream due to min>=0 flow variables and when it cannot?

I don't really understand the question,

In fact you gave me the required answer :)

but both frontends remove stream connectors with zero flow before generating inStream equations (which for outside connectors means max == 0 and for inside connectors min == 0). They don't do any constant evaluation, so they rely on the min/max attributes having been evaluated in an earlier phase.

Which may or may not be the case. Many libraries (e.g. Modelica.Fluid, ThermoPower, as well as the tests posted here) set the min attribute based on some parameter expression, which requires some evaluation.

The new frontend evaluates those attributes if they have constant bindings (i.e. is constant or structural parameter), the old frontend I think tries its best to always evaluate them.

OK, that's consistent with what I experienced. The old front end managed to carry out the symbolic simplifications in all the 10 cases I tried.

Now I removed the actualStream part of the model (see second attachment TestStreamConnectorsNoActualStream) to run the tests with -d=newInst and now I see the $OMC$PositiveMax macros in the dump of the first stages of the backend. They disappear as expected after simplifyInStream is executed.

comment:25 Changed 6 years ago by casella

The simplifications in tests Test1 through Test5 of TestInStreamNoActualStream.Tests, which contain unconnected connectors, one-to-one connections and a three way connection with a port with min = 0, are handled by the new front end correctly, as expected based on comment:23.

comment:26 Changed 6 years ago by casella

TestStreamConnectorsNoActualStream.Tests.Test6 has a component with this connector declaration

Interfaces.Flange inlet(m_flow(min = if allowFlowReversal then -1e9 else 0))

where allowFlowReversal is a parameter which is false by default. The new FE doesn't do any evaluation, and passes

pipe1.inlet.h_outflow = 
($OMC$PositiveMax(-pipe2.outlet.m_flow, 1e-007) * pipe2.outlet.h_outflow +
 $OMC$PositiveMax(-pipe3.inlet.m_flow, 1e-007) * pipe3.inlet.h_outflow) / 
 ($OMC$PositiveMax(-pipe2.outlet.m_flow, 1e-007) + $OMC$PositiveMax(-pipe3.inlet.m_flow, 1e-007))

to the back-end, where simplifyInStream turns it to

pipe1.inlet.h_outflow = 
(max(pipe2.inlet.m_flow, 1e-007) * source2.h +
 max(-pipe3.inlet.m_flow, 1e-007) * sink.h) / 
 (max(pipe2.inlet.m_flow, 1e-007) + max(-pipe3.inlet.m_flow, 1e-007)) 

which doesn't take into account the fact that pipe3.inlet.m_flow.min can be evaluated to zero because pipe3.allowFlowReversal = false, that allows to simplify the expression to

pipe1.inlet.h_outflow = 
 source2.flange.h_outflow = 
source2.flange.h

which is in fact the output of the old front-end.

The optdaedump reports

pipe3.inlet.m_flow:VARIABLE(flow=true min = if pipe3.allowFlowReversal then -1000000000.0 else 0.0 )  type: Real

so apparently simplifyInStream does not evaluate the min attribute and therefore does not recognize that min >=0, which would allow a further simplification.

Of course one could set annotation(Evaluate = true) on allowFlowReversal, so that the front-end does the processing. On the other hand people writing models may fail to recognize that this is necessary for the optimization to take place - the specification doesn't mention the need of setting Evaluate = true.

Since the removal of terms made irrelevant by min = 0 is more of an optimization, I am ok with performing the simplification in the back-end in this case. However, simplifyInStream should constant evaluate the min and max expressions, otherwise the thing doesn't work.

@vitalij, can you take care of this?

Thanks!

Changed 6 years ago by casella

comment:27 Changed 6 years ago by casella

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:28 Changed 6 years ago by vitalij

simplifyInStream don't force the evaluation of parameters.

We using simplify to evaluate the min/max values
https://github.com/OpenModelica/OMCompiler/blob/master/Compiler/BackEnd/BackendDAEOptimize.mo#L244

I would adress the topic evaluate parameter in some other module before call simplifyInStream e.g. EvalutateFinalParameters
with some additional flag e.g. forceEvalutionFlowAttributes or something else.

Note there maybe an site effect, because
-> simplifyInStream is after removeSimpleEquation (need alias merge)
-> maybe simplifyInStream can introduce new alias equation
-> simuliation/optimization/jacobian backend has an additional removeSimpleEquation but initialization-backend don't have, which maybe need most of all the attributes (like start) from the alias variables.

comment:29 Changed 6 years ago by vitalij

Of the other hand if you thinking about a heuristic like for tearing set where OM replace the parameter it's would maybe be possible direct if we can use the same function. But I like the solution with EvaluateParameter module more.

comment:30 Changed 6 years ago by vitalij

Simplify other Modelica tools like Dymola

pipe1.inlet.h_outflow = 
 source2.flange.h_outflow = 
source2.flange.h

comment:31 Changed 6 years ago by casella

I checked Modelica.Fluid.Interfaces.PartialTwoPort, and indeed the base class for two-port components contains the Evaluate annotation:

  parameter Boolean allowFlowReversal = system.allowFlowReversal
    "= true to allow flow reversal, false restricts to design direction (port_a -> port_b)"
    annotation(Dialog(tab="Assumptions"), Evaluate=true);

  Modelica.Fluid.Interfaces.FluidPort_a port_a(
      redeclare package Medium = Medium,
      m_flow(min=if allowFlowReversal then -Constants.inf else 0))

Since the MSL is meant as the standard reference for modelling style, I would say we can safely assume that if one wants this kind of optimization, he needs to tell the compiler explicitly to evaluate the parameter.

At this point, based on comment:23, most simplifications will be handled by the new front-end, because the min/max expressions will be constant evaluated.

The simplifyInStream function would then just be needed to handle those cases where the flow is set to zero without an explicit min = 0 declaration, as well as those where min/max propagation would be needed, for example if the min attribute is not set on the port variable but on an alias variable for the flow rate, or if there are two or more series-connected pipes and only one has allowFlowReversal=false, etc. In this case alias elimination and min/max merging is required to perform the simplification, which is a job for the back-end, not for the front-end.

I modified the test suite to include the Evaluate annotation on the allowFlowReversal parameter, see the attached TestStreamConnectorsNoActualStreamEvaluateParams.mo. I ran all the previous tests with the new front-end and they work as expected.

I then added one more Test11 with two series-connected pipes, of which the one involved in the three-way connection has allowFlowReversal=true, but the series-connected one has allowFlowReversal=false. In this case, as expected, the new front end does not perform any simplification, because it only looks at the min attribute of the connector involved in the three-way connection set, while simplifyInStream does it, by considering the limitation on the series-connected pipe via alias elimination and min/max merging.

Summing up, if one sets Evaluate = true on the parameters involved in the min/max attributes, the current situation is perfectly fine both with the old and new front-end. I will then re-close the ticket.

@perost, it would be nice if you added the TestStreamConnectorsNoActualStreamEvaluateParams.mo tests to the OMC testsuite. They include some asserts that check that critical stream variables are computed as expected. Can you please take care of that?

comment:32 Changed 6 years ago by casella

  • Resolution set to fixed
  • Status changed from reopened to closed

comment:33 Changed 6 years ago by vitalij

@perost,@casella I will add TestStreamConnectorsNoActualStreamEvaluateParams.mo as tests for simulation with +d=newInst to minimize the risk that I will broke something if I try to fix #5104

comment:34 Changed 6 years ago by casella

Fine for me. Let's just make sure that once newInst becomes the default and -d=newInst is no longer needed, these tests are still performed.

Note: See TracTickets for help on using tickets.