Adding yet another Complex record seems wrong

[modelica-trac-importer]

Reported by sjoelund.se on 24 May 2013 06:41 UTC
eeee36f introduced a new record Modelica.Media...MyRecord, which is not only wrong but seems to be a weird design choice.
First of all, import MyRecord means the record's name has to be .MyRecord, but it does not exist on the top level, so the code is wrong.
And we anyway have operator overloading and a Complex class. So why would we introduce yet another class that does the same thing?
If this is due to some tool not supporting operator overloading, [Note, the operator overloading as defined in this section is only a short hand notation for function calls.], which should mean you can use the operator record without implementing operator overloading, i.e.:

model M
  Complex compl1,compl2;
equation 
  compl1 = Complex.'constructor'(1,1);
  compl2 = Complex.'*'(compl1,compl1);
end M;

---

Migrated-From: https://trac.modelica.org/Modelica/ticket/1122

[modelica-trac-importer]

Comment by otter on 24 May 2013 07:14 UTC
Replying to [ticket:1122 sjoelund.se]:

eeee36f introduced a new record Modelica.Media...MyRecord, which is not only wrong

Sorry, there was a wrong import statement that I have just corrected in f8d78bf.

but seems to be a weird design choice.
First of all, import MyRecord means the record's name has to be .MyRecord, but it does not exist on the top level, so the code is wrong.
And we anyway have operator overloading and a Complex class. So why would we introduce yet another class that does the same thing?
If this is due to some tool not supporting operator overloading, [Note, the operator overloading as defined in this section is only a short hand notation for function calls.], which should mean you can use the operator record without implementing operator overloading, i.e.:

model M
Complex compl1,compl2;
equation 
compl1 = Complex.'constructor'(1,1);
compl2 = Complex.'\*'(compl1,compl1);
end M;

The used publication for the modeling of ice needed for ReferenceMoistAir used complex numbers for the (complicated) formulas. Hubertus suggested to not use Complex from MSL for that, because then tools not yet supporting operator overloading cannot handle Media. Since it seemed not easy to rewrite the equations to array notation, I made a copy of Complex and rewrote it so that it contains only a record with functions. The original Complex cannot be used because language elements "operator record" and "operator" are present, and a tool not supporting operator overloading will not support them. Stefan then used this class locally in ReferenceMoistAir and rewrote the equations. Here the mentioned small error appeared that I just fixed.

[modelica-trac-importer]

Comment by sjoelund.se on 24 May 2013 07:43 UTC
Martin, that is the main problem here. Why would we introduce yet another Complex class into MSL? It is unnecessary to add a duplicated class. Especially since tools that have not implemented operator overloading can still use the original Complex.mo if you call the functions explicitly instead of using the overloading. Just as I wrote in the example above.

Is there any good reason why Media could not use this approach? And anyway, Complex.mo has been part of the MSL for almost 3 years when we release 3.2.1. Surely that must be enough time for tools to support it. Note that the new Complex class was added as a public interface, which means you can not later remove it without breaking backwards compatibility.

[modelica-trac-importer]

Comment by otter on 24 May 2013 11:16 UTC
Replying to [comment:2 sjoelund.se]:

Martin, that is the main problem here. Why would we introduce yet another Complex class into MSL? It is unnecessary to add a duplicated class. Especially since tools that have not implemented operator overloading can still use the original Complex.mo if you call the functions explicitly instead of using the overloading. Just as I wrote in the example above.

No.

Complex is defined as:

operator record Complex
   ...
   operator '+'
      ...

and the language keyword "operator" is only supported if operator overloading is supported.

Is there any good reason why Media could not use this approach? And anyway, Complex.mo has been part of the MSL for almost 3 years when we release 3.2.1. Surely that must be enough time for tools to support it. Note that the new Complex class was added as a public interface, which means you can not later remove it without breaking backwards compatibility.

I have made MyComplex protected in 711fb85 (this required a minor rewriting, because a protected record containing encapsulated functions that import an absolute path are not correct).

[modelica-trac-importer]

Comment by sjoelund.se on 24 May 2013 11:29 UTC
Replying to [comment:3 otter]:

Complex is defined as:

operator record Complex
   ...
   operator '+'
      ...

and the language keyword "operator" is only supported if operator overloading is supported.

Not true. A tool only needs to change their parser to say that operator is the same as function and they have implemented that part. Operator overloading is only needed if you need to use the overloading aspects of the Complex record. It's just function calls.

Also, 711fb85 introduced a parsing error. And MyComplexF.'*', etc cannot be called since MyComplexF is protected and "A protected element, P, in classes and components may not be accessed via dot-notation".

[modelica-trac-importer]

Comment by otter on 24 May 2013 11:37 UTC
Replying to [comment:4 sjoelund.se]:

Replying to [comment:3 otter]:

Complex is defined as:

operator record Complex
...
operator '+'
...

}}}
and the language keyword "operator" is only supported if operator overloading is supported.

Not true. A tool only needs to change their parser to say that operator is the same as function and they have implemented that part. Operator overloading is only needed if you need to use the overloading aspects of the Complex record. It's just function calls.

As you write it: A tool has to change its parser. The goal is that a current tool that does not support overloading is able to process ReferenceMoistAir (without any parser changes).

Also, 711fb85 introduced a parsing error. And MyComplexF.'*', etc cannot be called since MyComplexF is protected and "A protected element, P, in classes and components may not be accessed via dot-notation".

In Dymola no error is reported, and I think Dymola is right. The structure is:

package Basic
  protected
    package MyComplexF
      function '+' 
      ...
    end MyComplexF
  public
    function Tsub
       ...
       T = MyComplex.'+'(....)

If this would not be allowed then this would also not be allowed:

model Test

protected
  function fun
    ..
  end fun;
equation
  T = fun(...)

[modelica-trac-importer]

Comment by beutlich on 24 May 2013 11:46 UTC
First example has dot-notation MyComplex.'+', second not.

[modelica-trac-importer]

Comment by sjoelund.se on 24 May 2013 11:47 UTC
Well. They don't have to change their parser. They could solve it by handling operator as if it was function. It should be a tiny change for any tool vendor to support that (I say that as OpenModelica supports operator overloading but not calling the operators in that way; it's a very, very, simple change).

This hack seems ugly as hell just to make sure the code works in tools that are >3 years old anyway. And keep in mind this is only to make old tools work with a newly introduced model anyway. While we have had Complex.mo in the library for soon 3 years. Why was that library added if people don't want to use it?

The model Test is different from the other one (doesn't use dot-notation and exists in the same scope as the protected function).

And you should still not close the ticket since Modelica 3.2.1/Modelica/Media/Air/ReferenceMoistAir.mo:2429 Error: Parse error: The identifier at start and end are different (doesn't Dymola check for this?).

[modelica-trac-importer]

Comment by otter on 24 May 2013 11:54 UTC
Replying to [comment:6 beutlich]:

First example has dot-notation MyComplex.'+', second not.

Adapted example:

model Test

protected
  package pack
     function fun
      ..
     end fun;
  end pack;
equation
  T = pack.fun(...)

Of course, this must be allowed. What is not allowed is an access Test.pack.fun.

[modelica-trac-importer]

Comment by hansolsson on 24 May 2013 12:06 UTC
Replying to [comment:5 otter]:

Replying to [comment:4 sjoelund.se]:

Not true. A tool only needs to change their parser to say that operator is the same as function and they have implemented that part. Operator overloading is only needed if you need to use the overloading aspects of the Complex record. It's just function calls.

As you write it: A tool has to change its parser. The goal is that a current tool that does not support overloading is able to process ReferenceMoistAir (without any parser changes).

Note that "process" is a key point: Modelica.Electrical.QuasiStationery, Modelica.Magnetic.FundamentalWave use the Complex class. A tool not supporting operator overloading will not be able to handle those packages either.

However, I don't understand the parsing bit: Complex is defined outside of MSL (using operator overloading).

Also, 711fb85 introduced a parsing error. And MyComplexF.'*', etc cannot be called since MyComplexF is protected and "A protected element, P, in classes and components may not be accessed via dot-notation".

In Dymola no error is reported, and I think Dymola is right. The structure is:

package Basic
  protected
    package MyComplexF
      function '+' 
      ...
    end MyComplexF
  public
    function Tsub
       ...
       T = MyComplex.'+'(....)

If this would not be allowed then this would also not be allowed:

model Test

protected
  function fun
    ..
  end fun;
equation
  T = fun(...)

Well, more importantly you would not be able to define a protected component and connect graphically to it:

protected 
...
  Inertia myIntertia;
equation
  connect(myInertia.flange_a, ...);

The key point is that you are not able to use ... .x to access the protected x - but you can access parts of x with x. ....

That is the general idea of using "protected" for encapsulation - protected contents is hidden from the outside, you have not hidden the protected contents from yourself!

(BTW: Dymola should check that the name after "end" is correct, but only generate a warning and automatically correct it.)

[modelica-trac-importer]

Comment by otter on 24 May 2013 12:13 UTC

(BTW: Dymola should check that the name after "end" is correct, but only generate a warning and automatically correct it.)

Thanks for this hint. I made the changes with an editor and when reading with Dymola I have overseen the warning (and Dymola could not store its changed version).

Fixed in d87b322 by correcting the typo in "end MyComplex".

Martin (S.) can you confirm that ReferenceMoistAir is now parsed by OpenModelica.

[modelica-trac-importer]

Comment by sjoelund.se on 24 May 2013 12:21 UTC
Confirmed
After reading Hans comments I'll concede that the code is probably valid right now. I still think it's a bad idea to duplicate the code.

Anyway, the comment about changing the parser is a quick hack to get operators running. If operator record is treated as a record, and operator (function) as function, you get the subset of operator overloading that does not require you to actually implement operator overloading (namely calling the individual functions like what was done in MyComplex).

You could even go so far as to create a "Complex nooverloading.mo" that did not even contain the operator keywords, etc, if you are a tool vendor that does not support operator overloading yet.

record Complex
  function 'constructor'
    ...
end Complex;

model M
  Complex c = Complex.'constructor'(1,0); // Would work in tools using both versions of Complex
end M;

But why not use Complex directly like the other packages already do?

[modelica-trac-importer]

Comment by otter on 24 May 2013 12:31 UTC
Replying to [comment:11 sjoelund.se]:

But why not use Complex directly like the other packages already do?

The other packages that use Complex (ComplexMath, ComplexBlocks, QuasiStationary, FundamentalWave) have been designed with Complex as basic building block (e.g. QuasiStationary is an implementation of stationary computation of electrical systems using complex numbers as used in the electrical literature).

I will now release MSL 3.2.1 Beta 1 with MyComplex. If the discussion reveals to go back to the "Complex" solution we can do that for the release candidate.

[modelica-trac-importer]

Modified by sjoelund.se on 20 Dec 2016 12:30 UTC