Improved controller of AST_BatchPlant example case

[casella]

This PR improves the controller of AST_BatchPlant by adding a suitable delay between the activation of the pumps and valves upstream V3 and V6 and their actual opening. This is more realistic, as it allows the pressure to stabilize, and it avoids a very short, but quite large, unrealistic backflow through those valves.

A bit of backflow is still present, but that cannot be eliminated unless the valve opening is a ramp and is not governed by a boolean signal.

This PR fixes #2686

[casella]

The PR was tested successfully with Dymola. OpenModelica still fails at time=2083, but at least produces the expected result before that.

We'll try to address the failure at time=2083, which I guess is due to extreme model stiffness, during the beta release period.

[beutlich]

It also simulates in SimulationX with the following two "non-zero" regions:

[HansOlsson]

As far as I can see the previously reported issue still remain.
(When storing with double precision and not storing equidistantly.)
If the problem is that an idealized controller with a detailed plant-model doesn't work I don't see the benefit of minor patches for it; either leave as is with a comment - or make everything realistic.

[dietmarw]

I had another look at this "fix" and as @HansOlsson correctly pointed out it does not solve the original issue. So as I understand it the issue occurs because a "real" model of water is controlled by an "ideal" valve. So what about instead of manipulating the sequences so that the model works in exactly this setup (which is not even the case and is also not useful as example for other users then) to use this as valve instead:

[casella]

@dietmarw, sounds good to me.

In fact, I wanted to do something similar, but I simply lacked the time to do that these days. I'll see what I can do tomorrow.

To be honest, I cannot guarantee that there won't be any reverse flow by doing this, but I expect the glitch to be really minor at that point. In which case, I hope the "fix" will be good for you guys 😄

[dietmarw]

Excellent. If you think it's of general interest and not just for that example you might want to place that new ValveRamp (maybe?) next to ValveLinear and ValveDiscrete.

[casella]

Good idea

[dietmarw]

@casella That "tomorrow" was now about 4 days ago ;-)

[casella]

Ok, I just followed @dietmarw's advice:

• I added a ValveDiscreteRamp model which has the same boolean input as ValveDiscrete, but opens gradually using a trapezoid signal generator.
• I used that model instead of ValveDiscrete in the AST_BatchPlant example
• I set the opening/closing times of the valves and the on/off times of the pumps, which were previously both zero, to realistic values: 0.1 s for the valves (solenoid-actuated) and 0.3 s for the pumps

Now the pressure profiles are correct, and the flows through V3 and V6 never get negative.

This hopefully fixes #2686 for good.

[dietmarw]

Looks good now.
@beutlich Still OK to merge for MSL 4.0.0? It would kill a bug.

[HansOlsson]

I cannot see that problems are gone, I still get a negative dip for V6.m_flow around 741s.

(Tested with Dymola 2020x; double precision, non-equidistant time-grid and tolerance=1e-7 just in case. And I see that it is declared using Modelica.Fluid.Valves.ValveDiscreteRamp).

Reran simulation with the parameters and issue is still present.

[dietmarw]

OK here my final review:
As @HansOlsson points out, yes there is still backflow present:

Now the question is, is this acceptable. For me I would say yes because:

1. With the improvements made they are now tiny and could almost be called numerical.
2. We are talking about an application example and not a component itself that is acting non-physical. Though the reason for this is, as @casella pointed out, due to the combination of ideal components with "real" media. This might not be always avoidable when doing modelling.
3. The reverse flow will now only be visible when switching to non-equidistant time-grid. (works fine with double precision). That is an option I've never even used before looking into this. So I guess it is not that relevant for the user to bump into when running this example.

I'd say it is definitely an acceptable improvement as to what there was before. And I would go so far that this PR now can close #2686 if not as "fixed" but as "worksforme". So I'd say the library officers @wischhusen and @hubertus65 should have the last word if this can be accepted or not.

[casella]

@dietmarw never mind bothering the other library officers. It turns out V3 and V6 had a small leak when closed, which is sometimes added to avoid numerical issues when you have multiple valves which are series-connected and are all closed, because the pressure in-between becomes undetermined.

In the case of V3 and V6 this was not really required, because the downstream pressure is fixed to the atmospheric value. I just reset opening_min to the default value of zero and even that tiny backflow is now gone, even with double precision and no equidistant time grid.

I hope this makes it work for you and for @HansOlsson and we can finally merge this PR into 4.0.0 😃

[hubertus65]

From the modeling point of view, I am fine with all changes. Here is my nitpicking: if I look at the result with this "numerical glitch" there, I can already smell the difficulties of obtaining numerically good enough equal results with another tool. We do that with all models in MSL, and OpenModelica is on the way there. The result in of V6.port_a.m_flow is a total nightmare to reproduce in another tool. Today it is not, but it should be our ambition for MSL testing. It may be possible to "hide" that glitch by an equidistant time grid (in several tools), but that is fragile and cheating. Both you @casella , and we at Modelon will burn unproductive hours to reproduce that crap. So the patch does not fulfill what should be our objective for MSL example models. The correct patch might be a different Medium model that fits better with basically incompressible assumptions, and this one just creates intermediate troubles. We should aim at example models that robustly run on multiple tools, this one will not. My perspective may be unique, but it shouls also be yours, Francesco @casella. @HansOlsson, I have an interesting challenge for you: produce results with both RadauIIa and Dassl in Dymola that compare as equal by cvscompare on this model. That is probably as difficult as with different tools. Have fun!

[casella]

I agree with @hubertus65 comments in general.

@hubertus65, I am convinced that the fixes I made make the model more robust w.r.t. different solvers and tools. You can test it, or take my word on it. Please just don't kill it in the cradle. There will be time during the RC1 period to test it with JModelica and report if you have problems with it.

[hubertus65]

So, I looked at this and the model is more robust than it has been, therefore it makes sense to approve the PR and move forward with this. I still fear this will be a very challenging model in its current form, if we want to achieve multi-tool testable example models. Anyways, I can see the argument that the perfect is the enemy of the good, the better even, in this case.

[HansOlsson]

@hubertus65 I agree that this model is numerically challenging, and it may be that we should re-design it.

My concern was that it shows non-physical effects without any explanation - and that people building similar real models would get similar issues; and wonder what is happening.

However, digging deeper regarding actual models I now realize that Modelica.Fluid.Examples.AST_BatchPlant.BaseClasses.TankWithTopPorts does not check the flow at top-ports, whereas Modelica.Fluid.Vessels.OpenTank does (I assume there is some deeper explanation for this). It seems unlikely that people directly use Modelica.Fluid.Examples.AST_BatchPlant.BaseClasses.TankWithTopPorts - in their own models, so I think this is likely fine now.

[wischhusen]

I agree with @hubertus65. Moreover, I think we defined a bandwidth for differences in regression testing somewhere. I think that this quality measure should also hold to differences in tool result. In my personal opinion the potential for tool differences must be very low now and certainly below that limit.

In the end, this model is a demonstrator and not used for design of batch plants. On the other hand people could come across the idea to reuse the base models for their purposes. Hence it would be required to check first if the base models can either substituted by the "standard" models of the Fluid. If not, potential failures (if any) must be corrected.