Avoid function names with single-quoted idents in ComplexMath

[beutlich]

There is some work left to do for tool vendors since the converted test model ModelicaTestConversion4.ComplexMath.Issue883 aborts the translation for various reasons.

model Issue883 "Conversion test for #883"
  extends Modelica.Icons.Example;
  import 'abs' = Modelica.ComplexMath.abs;
  parameter Complex c1 = Complex(sqrt(2)/2, sqrt(2)/2);
  parameter Complex c2 = Complex(-sqrt(2)/2, -sqrt(2)/2);
  Real r = 'abs'(c1);
  Complex c3=Modelica.ComplexMath.sqrt(c1);
  Complex c4=Modelica.ComplexMath.max({c1,c2});
  Complex c5=Modelica.ComplexMath.min({c1,c2});
  Complex c6=Modelica.ComplexMath.sum({c1,c2});
  Complex c7=Modelica.ComplexMath.product({c1,c2});
  Real n=Modelica.ComplexMath.Vectors.norm({c1,c2});
annotation(experiment(StopTime=1), uses(Modelica(version="4.0.0-dev")));
end Issue883;

@HansOlsson Dymola 2020 has an issue with the modified function Modelica.ComplexMath.Vectors.norm and reports

Unimplemented: No support for unexpanded array of records.
Internal error in code generation, please contact support.

If that line is removed the model simulates as expected.

@gkurzbach SimulationX 4.0.2 reports singular systems (error Check.Singular) for the variables c4.re, c4.im, c5.re, c5.im, i.e., the renamed max and min functions in Modelica.ComplexMath. If these two lines are removed from the model it simulates as expected.

@christiankral I did not try OpenModelica 1.14.

@harmanpa FYI

closes #883

[sjoelund]

I tried this in OpenModelica. It does not like reductions using .sum(... for ... in ...). Reading the specification (10.3.4.1) it seems to suggest you can only use the names directly and no lookup is performed on them...

[beutlich]

Does this mean we cannot fix #883 according to spec 3.4?

[sjoelund]

Well, I think it can be fixed according to the spec (just skip the leading the dot). But then Dymola and SimulationX might have problems because they find a different sum function. I also don't think it would be a big change in OpenModelica to allow the code in this PR, but then the specification should be clarified...

[beutlich]

Hm, I am confused. In modelica/ModelicaSpecification#880 (comment) @perost reported that it (i.e., abs with a leading dot = .abs) works in OpenModelica as expected.

[perost]

Hm, I am confused. In modelica/ModelicaSpecification#880 (comment) @perost reported that it (i.e., abs with a leading dot = .abs) works in OpenModelica as expected.

@sjoelund is talking about reduction expressions, i.e. sum(... for ... in ...), which aren't mentioned in the issue you linked to as far as I can see.

[beutlich]

Hm, I am confused. In modelica/ModelicaSpecification#880 (comment) @perost reported that it (i.e., abs with a leading dot = .abs) works in OpenModelica as expected.

@sjoelund is talking about reduction expressions, i.e. sum(... for ... in ...), which aren't mentioned in the issue you linked to as far as I can see.

Hence my confusion, since I interpreted that comment rather generously that built-in operators with a leading dot work in OM, no matter if it is a reduction operator or not.

[perost]

@beutlich: I'm pushing in support for fully qualified reduction expressions in OM now. The specification might technically not say that it's allowed, but I suspect that's just an oversight. Our current frontend used to give a rather confusing error message in that case, while our new frontend allowed it. So I decided to just allow it everywhere to be consistent.

[beutlich]

@henrikt-ma The above test model also fails in Wolfram SystemModeler. Can you have a look at it?

[1] 14:14 Validation of model Issue883 
[19:5:20-3] Error: Expected a reduction function with type signature ('A, 'B) => 'B, but got .sum<function>() => #NORETCALL#.
[0:0:0-0] Error: Error occurred while flattening model Issue883
Check failed for model: Issue883.

[sjoelund]

@beutlich that's the behaviour of OpenModelica (which WSM still has parts of). Easy to fix, if the specification is clarified...

[gkurzbach]

@gkurzbach SimulationX 4.0.2 reports singular systems (error Check.Singular) for the variables c4.re, c4.im, c5.re, c5.im, i.e., the renamed max and min functions in Modelica.ComplexMath. If these two lines are removed from the model it simulates as expected.

I confirm that this is a tool issue. The code itself is fine. I will be fixed in next release.

[henrikt-ma]

I have nothing to say in addition to what @sjoelund has already said.

[beutlich]

@henrikt-ma Are you going to fix it in Wolfram SystemModeler?

[henrikt-ma]

@henrikt-ma Are you going to fix it in Wolfram SystemModeler?

Yes.

[HansOlsson]

Looks good now.

[beutlich]

@HansOlsson Dymola 2020 has an issue with the modified function Modelica.ComplexMath.Vectors.norm

@HansOlsson Please confirm the tool-issue such that I know you'll be going to fix it. Thanks.

[HansOlsson]

@HansOlsson Please confirm the tool-issue such that I know you'll be going to fix it. Thanks.

Yes, as far as I can see that tool-issue is fixed.
However, I tried to check the entire MSL to ensure that there were no other issues and stumbled on
Modelica.Blocks.Examples.Modulation containing:

Modelica.Blocks.Sources.Sine amplitude(
amplitude=0.5,
freqHz=2,
offset=1)
but the Sine-block has 'f' not 'freqHz' as parameter.

I assume that issue is unrelated, and I could continue check - but it will be hard to know what issues are related to this PR and which ones are unrelated.

[HansOlsson]

(To be clear, there was an issue and it was corrected for the next release. However, I haven't verified that is the only issue.)

[sjoelund]

Using .sum in reductions isn't clarified even in the specification master branch, right? It should be fixed there before it's used in MSL (doesn't need to be released as long as there is consensus and we stage it for next release as a clarification).

[HansOlsson]

Using .sum in reductions isn't clarified even in the specification master branch, right?

Yes, that is a remaining issue with this PR. However, I view that as a change (or extension) not a clarification.

The specification currently requires that the function-name is one of array, sum, product, min, and max. We could also allow ".sum" - but on the other hand it shouldn't be needed; the reduction-operators rely on certain names - not function lookup leading to those names.

[henrikt-ma]

@HansOlsson: That's a good point. The leading . shouldn't be needed, and I think it would be better to clarify this in the specification and make tools implement this, than going in the direction of allowing a redundant ..

[sjoelund]

I think that might be a bad direction if we want future additional reductions added (or user-defined reductions).

[henrikt-ma]

As I see it, the sum array reduction isn't the same as the built-in sum function. One is an array reduction while the other is a function. When sum is used as a key to select one of the array reductions, I see the identifier sum as the key, not the built-in function bound to sum.

If we want user-defined reductions I think it would be better to just add a fold reduction, perhaps like this:

fold(fun, nullResult, e(i, …, j) for i in u, …, j in v)

Here, we don't need lookup for fold (which doesn't even exist as a built-in function), only for fun and nullResult.

This way, functions don't have to deal with the for iterator stuff for reductions. To users, it might be a bit confusing that they share parts of the grammar (function-arguments), but to me that's just how the grammar happens to be written in the specification. Tools will still be able to keep the two unrelated concepts apart.

[HansOlsson]

I tried this in OpenModelica. It does not like reductions using .sum(... for ... in ...). Reading the specification (10.3.4.1) it seems to suggest you can only use the names directly and no lookup is performed on them...

Clarified: We will ensure that sum(... for .. in ...) works in the coming release of Dymola as well; regardless of lookup of sum (and keep .sum for compatibility reasons).

However, I also found an inconsistency related to this.
Modelica.ComplexMath.Vectors.norm uses the reduction expression .sum with an iterator expression.
Modelica.ComplexMath.Vectors.length uses the built-in .sum-function on an array, which is built using an array reduction expression.

If we agree that the second variant should work as specified with .sum (and it already works in tools) we could use it, even if slightly clumsy - if possible.
The latter is important as .sum(f(i) for i in 1:n)) and .sum({f(i) for i in 1:n) are only the same if f(i) is a scalar expression.

We can later consider adding more general reductions, e.g. using fold as suggested by @henrikt-ma
And possibly consider both foldl and foldr, etc.
https://en.wikipedia.org/wiki/Fold_%28higher-order_function%29 states that C++17 uses the syntax of: (... op arr) (foldl) and (op arr ...) (foldr).

[beutlich]

Can this be merged as is?

[svorkoetter]

Looks fine to me. MapleSim had no issue with .sum

[claassistantio]

All committers have signed the CLA.