Fix DynamicSelect for new API

[OpenModelica-TracImporter]

No description provided.

[mwetter]

The model Buildings.Fluid.FixedResistances.Examples.FlowJunction should look like this:

In OMEdit, this model looks like this:

Other models however work. For example, Buildings.Controls.OBC.CDL.Continuous.Validation.Abs shows the DynamicSelect

[casella]

@mwetter, thanks for the report. @perost will investigate and try to fix this.

[perost]

The issue with FlowJunction is that is uses DynamicSelect("", String((if use_T_in then T_in else T) - 273.15, format = ".1f")), and we only kind of handle the other overload of String(Real).

So this is mostly an OMEdit issue, the frontend can return whatever format we want but OMEdit needs to handle it somehow. @adeas31, any comments?

[casella]

we only kind of handle the other overload of String(Real).

That is?

I'm not sure I understood what the problem is.

[perost]

we only kind of handle the other overload of String(Real).

That is?

I'm not sure I understood what the problem is.

There are two overloads for String(Real):

 String(r, significantDigits=d, <options>)
 String(r, format=s) 

We only kind of handle DynamicSelect with the first overload (we use significantDigits but ignore the other options), but not the second overload.

There's also the added issue that the first argument in the String call is an expression and not just a variable name, which I don't know how OMEdit should handle. Since the frontend doesn't know what to do with this it just returns the first argument, in this case an empty string.

[adrpo]

To properly fix this we need an expression parser/evaluator in OMEdit. We already have the ANTLR4 Modelica parser linked with OMEdit. I have started working on the evaluator but had no time to finish it. I'll see if I can dig out the code.

Basically OMEdit should be able to evaluate the DynamicSelect expression by looking up the component names in the result file (or the model init file for parameters). OMC should constant evaluate all the constants (or the stuff that doesn't end up in the result files / model init file). This will handle most of the cases but not all.

I think that to make sure we handle all the cases we would need to sent the annotations to OMEdit and evaluate them there as DynamicSelect can be used anywhere in the annotations. OMC should qualify the component names of the components that appear in the annotations so that OMEdit can find them in the init files or the result file.

[casella]

To properly fix this we need an expression parser/evaluator in OMEdit.

This would also finally resolve the long-standing issues #2081 and #2661, which would be really good. The first in particular is a major obstacle to proper usability of OMEdit with libraries that use the construct, first of all MSL that usese them extensively since version 3.0.

Although it would be nice to close this ticket by the end of September, we have time to do that until the end of the year. If the task can be handed over to Per, maybe we have a chance of getting it done by then.

BTW, we don't need a particularly sophisticated expression evaluator, the kind of expressions that are used as dynamicSelect or Dialog annotation are usually very simple.

[casella]

In any case, it would probably be good to support all five variants of Strings(), eventually.

[perost]

In any case, it would probably be good to support all five variants of Strings(), eventually.

That would be solved if OMEdit could evaluate expressions. The arguments of each String overload uniquely identify which overload it is though, so if we need it sooner we can just pass the arguments as a list and let OMEdit figure out what to do. I was going to implement that when I realised it wouldn't help since the first argument is an expression that OMEdit can't evaluate.

[casella]

OK. The question then is what is the status of @adrpo's code and if it can be taken over easily by someone else.

A possible workaround on the library side is that we define additional variables/parameters to compute those expressions, and then just use their names in the dynamicSelect annotations, Dialog annotations, and conditional component activation conditions, so that evaluating them would be trivial.

The problem is, we can't really do that with MSL, which is alread released, and it may not be particularly convenient for the user, so I would rather avoid that.

Maybe one possible solution is to have the NF automatically do that, i.e. generate auxiliary variable/parameter definitions with binding equations corresponding to those expressions, and then using the aux names in the dynamicSelect, Dialog and conditional component activation expressions. For example

DynamicSelect("", String((if use_T_in then T_in else T) - 273.15, format = ".1f"))

would be transformed into

String $aux1 = String((if use_T_in then T_in else T) - 273.15, format = ".1f");
DynamicSelect("", $aux1);

and

parameter Boolean p1;
parameter Boolean p2;
MyConnector c if p1 and not p2;

would become

parameter Boolean p1;
parameter Boolean p2;
parameter Boolean $aux1 = p1 and not p2;
MyConnector c if $aux1;

etc.

Evaluating them would become trivial for OMEdit. For parameters, there could be an API doing that by (partial?) instantiation. For variables, you just need to pick them in the result file and display them properly.

Seems rather straightforward to implement to me.

@perost, @adrpo, what do you think?

[perost]

@perost, @adrpo, what do you think?

It would be trivial if we only had to change the flattened model, but I'm not sure how it would work in the API. We'd have to change the Absyn, which I guess means changing the user's model too?

[casella]

@perost, sorry but I don't get your point. I understand the currently available implementation of dynamicSelect works if the quantity to be displayed is given by a variable or parameter, but it doesn't if it is given by an expression, because OMEdit cannot evaluate it. Is that correct?

In that case, we simply define an additional variable or parameter bound to the original expression, that will be displayed as other "native" non-expression values. Why is it required to change Absyn?

[perost]

@perost, sorry but I don't get your point. I understand the currently available implementation of dynamicSelect works if the quantity to be displayed is given by a variable or parameter, but it doesn't if it is given by an expression, because OMEdit cannot evaluate it. Is that correct?

Yes

In that case, we simply define an additional variable or parameter bound to the original expression, that will be displayed as other "native" non-expression values. Why is it required to change Absyn?

Because the annotations are not in the flat model, they're in the parsed Absyn. OMEdit fetches the annotations by using the API to query the Absyn model, so we would need to actually change the user's model to make it work.

[casella]

I'm sorry, I'm still not sure I get it. If the NF defined additonal variables bound to the original expressions, and then uses then in the annotations instead of the original expressions, in a sense we are changing the user's model.

So be it, as long as we do that automatically and without changing the model source code on mass storage. Is that a problem?

[perost]

I'm sorry, I'm still not sure I get it. If the NF defined additonal variables bound to the original expressions, and then uses then in the annotations instead of the original expressions, in a sense we are changing the user's model.

So be it, as long as we do that automatically and without changing the model source code on mass storage. Is that a problem?

The issue is that the NF is not really involved. When using the nfAPI we instantiate the annotations using the NF, but only the annotations. For this to work we'd have to change the Absyn after parsing the model, which will also change what OMEdit shows to the user unless we try to hide it somehow.

I don't really see how to implement that in a sane way, so I will just implement some expression evaluation in OMEdit instead. It's not actually that complicated to do.

[casella]

The issue is that the NF is not really involved. When using the nfAPI we instantiate the annotations using the NF, but only the annotations.

OK. So you just get the expression and that's it. I thought that in these particular cases (dynamicSelect, conditional connectors) we could instantiate the entire model - I thought this was not particularly efficient, but it is already implemented. In fact, it's not really implemented either.

I don't really see how to implement that in a sane way, so I will just implement some expression evaluation in OMEdit instead. It's not actually that complicated to do.

LGTM. I just wanted to avoid duplicate work, possibly at the cost of slower performance. That can be improved later on.

However if that becomes insane, go for the expression evaluator, you are in a much better position to judge on sanity and on trade/offs than I am in this case 😃

[perost]

To keep you up to date I've now implemented an expression class that can parse and evaluate flat modelica expressions and supports most of the builtin operators and functions. There are a few gotchas due to flat modelica lacking type information and all variables are treated as Real, but hopefully that won't cause too many issues. My code is currently written as a standalone program to make it easier to iterate, but I will start integrating it into OMEdit on monday.

One question I have though is how DynamicSelect is actually supposed to work? The specification talks about editing state and non-editing state but doesn't explain what that is. It seems like we don't do anything special with it in OMEdit, but maybe I've missed something?

[adrpo]

What I understand is:

• the first is used when showing and editing the diagram and it needs to be constant evaluated to a literal (Modeling perspective, diagram)
• the second will apply dynamic changes (based on the simulation results) on the non-editable diagram (Plotting perspective, diagram)

[adrpo]

Basically OMEdit will get the entire annotation as it is but with all the names prefixed properly:

original annotation:

annotation (
  Icon(graphics={Rectangle(
     extent=DynamicSelect({{0,0},{20,20}},{{0,0},{20,level}}),
     fillColor=DynamicSelect({0,0,255}, if overflow then {255,0,0} else {0,0,255}))}
  );

prefixed annotation:

annotation (
  Icon(graphics={Rectangle(
     extent=DynamicSelect({{0,0},{20,20}},{{0,0},{20,prefix.with.what.is.needed.level}}),
     fillColor=DynamicSelect({0,0,255}, if prefix.with.what.is.needed.overflow then {255,0,0} else {0,0,255}))}
  );

When displaying the diagram in the Modeling perspective it will use the first argument of DynamicSelect.
When displaying the diagram in the Plotting perspective will evaluate the second argument of DynamicSelect and change the diagram accordingly.

[adeas31]

And in the non-editing state i.e., diagram in plotting perspective, you can animate the diagram using the slider and the play controls above the variables browser.

[perost]

I've now made PR #7946 that implements expression parsing/evaluation for handling DynamicSelect. There are still some issues, see my comment in that PR, but I can now simulate Buildings.Fluid.FixedResistances.Examples.FlowJunction and the values in the diagram window will then update as I move the time slider.

[casella]

Great! As soon as we have it in the nightly we'll ask for feedback from LBL

[perost]

The PR is now merged in, so it should be in tomorrow's nightly build. There are still some issues to resolve so I will continue working on it a bit more, but the basic functionality should be there at least.

[casella]

@mwetter, can you try this out on the nightly build starting tomorrow and report?

Thanks!

[mwetter]

@casella : Yes, I will do tomorrow Thursday.

[mwetter]

I don't see any changes to what is reported in the figure of #5631 (comment).
This is with 1.19.0~dev-232-g2af9139 on Ubuntu 18.04.

[sjoelund]

OK, but I'll update the autoconf stuff anyway

[casella]

On Windows now it looks like this

The icons with the DynamicSelect annotations are pitch black. @adeas31, @perost, any idea what is still missing?

[perost]

The icons with the DynamicSelect annotations are pitch black. @adeas31, @perost, any idea what is still missing?

I've only implemented it for textString so far, I will change the rest of the annotation attributes to handle DynamicSelect next week.

[adeas31]

That is because we now return DynamicSelect as it is and that is only handled for textString attribute yet. The ellipse icons are black since the fillColor attribute has DynamicSelect in the the annotation and there is no support for it yet so the default black color is color.

[perost]

On second thought it's easy to just remove the DynamicSelect calls and use the first argument for annotations that don't yet support it, so I did that in #7967. That way we at least don't break annotations until we implement full support for DynamicSelect.

[casella]

OK, thanks!

[perost]

It looks like the Linux nightly builds are working again, and the latest one should be up to date with the DynamicSelect improvements.

[casella]

OK, with the latest nightly the right static icons are back:

I stand by for the complete implementation, then.

[perost]

I found an issue that I feared might show up, which is that some parameters used in the annotation does not show up in the result file (in this case bouX.use_T_in). OMEdit just returns 0.0 if it can't find a variable, which is bad since it means it works but can give the wrong result (it's happens to be correct for FlowJunction though since use_T_in is always false, which is what 0.0 is interpreted as).

I'm not sure what determines whether a parameter is recorded in the result file or not. The ones I've seen have HideResult=true which might be what's causing the issue. We could make sure any such parameters are evaluated by the frontend when dealing with annotations, but that's not guaranteed to work since it can only evaluate fixed parameters.

[casella]

I found an issue that I feared might show up, which is that some parameters used in the annotation does not show up in the result file (in this case bouX.use_T_in). OMEdit just returns 0.0 if it can't find a variable

I guess it should give NaN instead, unless there are good reasons to use this default value. It seems a bit dangerous to me.

which is bad since it means it works but can give the wrong result (it's happens to be correct for FlowJunction though since use_T_in is always false, which is what 0.0 is interpreted as).

See above.

I'm not sure what determines whether a parameter is recorded in the result file or not. The ones I've seen have HideResult=true which might be what's causing the issue.

Certainly, it is set in here. By default also protected variables are not saved, but there is a flag -emit-protected to override that.

We could make sure any such parameters are evaluated by the frontend when dealing with annotations, but that's not guaranteed to work since it can only evaluate fixed parameters.

I guess in most if not all cases these parameters have some kind of structural value. In this specific case there is actually an Evaluate = true annotation, but even absent that, this parameter is used to activate conditional components, so it can't for sure be changed at runtime.

So, I'm fine with a solution that evaluates them and makes the frontend return their numerical value.

[perost]

Support for DynamicSelect in more annotation attributes have now been implemented in #7975, and #7976 should fix the issue that some structural parameters weren't evaluated.

There's still room for improvement, DynamicSelect support is still missing for any of the enumeration values (like fillPattern) and for selecting things like fonts or images. But I need to work on other issues, and the new API should now support the same DynamicSelect features that the old one does (and much more) which is what this issue was about.

[casella]

Support for DynamicSelect in more annotation attributes have now been implemented in #7975, and #7976 should fix the issue that some structural parameters weren't evaluated.

Bingo!

There's still room for improvement, DynamicSelect support is still missing for any of the enumeration values (like fillPattern) and for selecting things like fonts or images. But I need to work on other issues, and the new API should now support the same DynamicSelect features that the old one does (and much more) which is what this issue was about.

Sounds like a good plan. @mwetter, if you need DynamicSelect support on enums, please open a separate ticket.

[mwetter]

Great to see it working! I will test it once apt-get works again for omc-common in the nightly builds (already reported on separate thread).

[mwetter]

@casella : I get a very different image for this model:

This is with 1.19.0~dev-266-g04c84c7 (updated today).
Interestingly, looking at bou1, we have -556.3+273.15+273.15 = -10, while the correct display should be +10. The Modelica statement is

DynamicSelect("", String((if use_T_in then T_in else T)-273.15, format=".1f"))

[perost]

@casella : I get a very different image for this model:

This is with 1.19.0~dev-266-g04c84c7 (updated today). Interestingly, looking at bou1, we have -556.3+273.15+273.15 = -10, while the correct display should be +10. The Modelica statement is

DynamicSelect("", String((if use_T_in then T_in else T)-273.15, format=".1f"))

That's very bizarre, is the simulation result for e.g. bou1.T correct? Can anyone else replicate this?

[mwetter]

The time series are corrected when plotted as a graph in OMEdit, just the schematic model view shows these wrong results.

[perost]

@mwetter: I guess the most likely is that it's doing -T - 273.15 instead of T - 273.15, which would result in e.g. -556.3 for bou1 since bou1.T is 283.15. But I don't know how that could possibly happen given that it works correctly for me and casella.

Can you please check what happens if you change the annotation to just:

DynamicSelect("", String(T, format="1.f"))

[mwetter]

@perost : With

DynamicSelect("", String(T, format="1.f"))

I get the correct display.

(Tested using OpenModelica 1.19.0~dev-271-gb6f3a96 which also reproduces the wrong display reported in #5631 (comment))

[casella]

Is it perhaps a Fahrenheit reading? 😄

@mwetter this is really strange, any of your collegues reporting the same issue?

[perost]

@mwetter: I was just going to comment that I installed Ubuntu in a VM and still couldn't reproduce the issue when it hit me: you're using the old API which for some reason is broken. Go to Tools->Options->General, then scroll down to the bottom and check "Enable new frontend use in the OMC API".

As a bonus this will also significantly improve the performance of the GUI, I almost fell asleep waiting for FlowJunction to open with the old API.

I will check if it's possible to fix the issue with the old API without too much effort, but it shouldn't really be used anyway.

[mwetter]

@perost : Thanks, I wasn't aware that I still have the old API for the front end enabled. This solves it, and it solves the slow GUI response.

[perost]

The issue with the old API is solved in #7995, and with that I think we can finally close this issue.

The problem was that e.g. T - 273.15 was "simplified" by the old frontend to -273.15 - T, which due to a mistake in the Expression class was parsed as -(273.15 - T).