search

modelica / Modelica_LinearSystems2

Free library providing different representations of linear, time invariant differential and difference equation systems, as well as typical operations on these system descriptions.
19 stars 19 forks source link

Remove "Modelica_" prefix from Modelica_LinearSystems2 library #32

Open modelica-trac-importer opened 7 years ago

modelica-trac-importer commented 7 years ago

Reported by dietmarw on 11 Sep 2015 07:51 UTC The latest release v2.3.3 of Modelica_LineraSystems2 contains a change described in the release notes as:

The following functions are not correct Modelica because local and result arrays depend on dimensions defined locally (but must be from inputs or parameter expressions). Since this is not practical to fix, the Dymola specific annotation __Dymola_allowForSize=true was introduced in Dymola 2016 FD01 to allow relaxed rules for Modelica dimension definition: * StateSpace.Import.fromFile * StateSpace.Import.fromModel * Internal.StateSpace2.Import.fromFile * DiscreteStateSpace.Import.fromFile * DiscreteStateSpace.Import.fromModel

This basically means that it has become less compatible with Modelica Specification and it it being included in the MSL seems now even more unlikely than it was before.

Now the idea of Modelica_* named libraries used to be libraries which are supposed to be included into the MSL soonish. This I think no longer holds for Modelica_LinearSystems2 and I suggest the name should simply be changed to LinearSystems2 or maybe even simply LinearSystems. It would still reside in the "group" of MA maintained libraries but simply not make any specification-conform promises which come with the name Modelica_*

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

modelica-trac-importer commented 7 years ago

Modified by dietmarw on 11 Sep 2015 08:36 UTC

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 11 Sep 2015 09:46 UTC Replying to [ticket:1782 dietmarw]:

The latest release v2.3.3 of Modelica_LineraSystems2 contains a change described in the release notes as:

The following functions are not correct Modelica because local and result arrays depend on dimensions defined locally (but must be from inputs or parameter expressions). Since this is not practical to fix, the Dymola specific annotation __Dymola_allowForSize=true was introduced in Dymola 2016 FD01 to allow relaxed rules for Modelica dimension definition: * StateSpace.Import.fromFile * StateSpace.Import.fromModel * Internal.StateSpace2.Import.fromFile * DiscreteStateSpace.Import.fromFile * DiscreteStateSpace.Import.fromModel

This basically means that it has become less compatible with Modelica Specification and it it being included in the MSL seems now even more unlikely than it was before.

To clarify: The new library version is not less compatible with the specification than before, in fact some incompatibilities are gone (*) .

Basically the sizes of outputs in these function depend on local variables, which is not correct Modelica. That problem was neither added nor corrected in the new version.

Modelica Specification states that an output size in a function may according to 12.2 only depend on parameters, constants, and "input formal parameters", and 12.4.4 (a bit implicitly) state that only public inputs are "input formal parameters".

What has changed in this library update is that instead of relying on an undocumented "feature" in Dymola ("protected input" was in some cases treated as "input formal parameters") the library now makes that tool-dependency clear. The reason for the change is that we started removing that undocumented "feature" in Dymola; so the old library now correctly generates warnings.

If there are other tools handling the old library without warnings it should be reported as a bug to them.

The question is then how to make the library efficient and Modelica compliant. A complicating factor is that these sizes are used as dimensions for connector-arrays.

(*): In addition there were function calls giving values to those protected inputs in the previous version. (I believe it was always the same as the default one.) That was corrected.

modelica-trac-importer commented 7 years ago

Comment by otter on 11 Sep 2015 15:34 UTC Some quick background information: I did not know that this part of Modelica_LinearSystems2 is not Modelica compliant (a DLR colleague made these models many years in the past). With the new Dymola Beta release new warnings occured complaining of non-Modelica compliant code. I fixed these issues several months ago. By a final test this week, one (completely unrelated) model gave an error and I was not able to figure out the reason. With Hans help it turned out that this model reads a linear system description from file and the dimensions of the system matrix define the sizes of input and output connectors. Due to the stricter checking of the new Dymola prototype, Dymola cound not figure out the dimension of these connectors and rejected to make a connection.

I discussed with Hans what to do, and all (short term) options are not good. I agreed with Hans to make this issue explicite by using a vendor-specific annotation (__Dymola_allowForSize=true), instead of re-introducing a questionable modeling construct (protected input) and rely on a heuristic of Dymola.

The issue is here that in a Modelica function data is read from file and based on this data 28 array dimensions are declared in this function (Modelica_LinearSystems2.StateSpace.Import.fromFile). From my point of view this is quite similar as to get the dimension from input arguments and the only reasonable solution for me is to enhance the Modelica standard so that such cases can be handeled.

modelica-trac-importer commented 7 years ago

Comment by stefanv on 11 Sep 2015 17:04 UTC Replying to [comment:3 otter]:

I discussed with Hans what to do, and all (short term) options are not good. I agreed with Hans to make this issue explicite by using a vendor-specific annotation (__Dymola_allowForSize=true), instead of re-introducing a questionable modeling construct (protected input) and rely on a heuristic of Dymola.

In other words, you have explicitly added an annotation that ensures that this function works only in Dymola, and also violated the rule that an annotation cannot affect the semantics (working vs. not working is different semantics IMO).

From my point of view this is quite similar as to get the dimension from input arguments and the only reasonable solution for me is to enhance the Modelica standard so that such cases can be handeled.

Getting the dimensions from a side effect is in no way similar to getting the dimensions from an input argument. If you were to allow this, then you should also allow functions that return arrays of random size.

Allowing this to be handled means that a tool must be able to change the number of equations in a model, arbitrarily, at simulation time. That does not seem like a "reasonable solution" to me.

modelica-trac-importer commented 7 years ago

Comment by otter on 13 Sep 2015 17:25 UTC Replying to [comment:4 stefanv]:

Replying to [comment:3 otter]:

I discussed with Hans what to do, and all (short term) options are not good. I agreed with Hans to make this issue explicite by using a vendor-specific annotation (__Dymola_allowForSize=true), instead of re-introducing a questionable modeling construct (protected input) and rely on a heuristic of Dymola. __

In other words, you have explicitly added an annotation that ensures that this function works only in Dymola, and also violated the rule that an annotation cannot affect the semantics (working vs. not working is different semantics IMO).

The alternative would have been to remove this function from the Modelica_LinearSystems2 library and some other functions and models as well (e.g. the analyze function linearizes a Modelica model, reads the linearize data from file and then performs analysis on it). As a result, important functionality would no longer be available and existing user models would no longer work.

Note, the Modelica_LinearSystems2 library is listed at https://www.modelica.org/libraries under "Other libraries developed by MA" with "not yet standard conform". In the documentation of the library it is stated under Modelica_LinearSystems2.UsersGuide.Requirements

This library is implemented with Modelica 3.1 (especially the operator overloading features are utilized) and requires the LAPACK 3.1.1 object library. Furthermore, linearization and plotting is implemented with Dymola API calls. It is planned to move these tool specific calls to the ModelicaServices package (introduced for the Modelica Standard Library 3.1 for these purposes), in order that other Modelica tools can provide the same functionality in a clean way.

According to my understanding, currently only Dymola is using this library. In the past there have been some discussion with Peter F. regarding support for OpenModelica, but I do not know the status.

To summarize, a non-Modelica compliant feature was detected in Modelica_LinearSystems2 and this was made explicite with a vendor-specific annotation.

Many parts of the Modelica_LinearSystems2 library are important for controller people. I have moved in the past some urgently needed functionality from Modelica_LinearSystems2 library to the Modelica Standard Library (such as Modelica.Math.Matrices.continuousRiccati/discreteRiccati), but of course would prefer not such an ad-hoc approach, and would like to utilize the many nice features in a standardized way.

From my point of view this is quite similar as to get the dimension from input arguments and the only reasonable solution for me is to enhance the Modelica standard so that such cases can be handeled.

Getting the dimensions from a side effect is in no way similar to getting the dimensions from an input argument. If you were to allow this, then you should also allow functions that return arrays of random size.

Allowing this to be handled means that a tool must be able to change the number of equations in a model, arbitrarily, at simulation time. That does not seem like a "reasonable solution" to me.

Dymola is not doing this. Instead when translating a model the file is read and with this information the model is built. After translation of the model, the file is no longer inspected.

I did not think of this problem previously, but this feature is used also in other cases: For example, in the commercial DLR FlexibleBodies library, the information about a flexible body is read from file and this file was generated from a Finite-Element program. Again, the dimensions of vectors/matrices in a Modelica model depend on sizes of arrays on a file, and then this must also be not Modelica compliant.

There are also other commercial libraries in this field (especially for flexible bodies) from other organizations, and they all have the same problem (but this was not recognized, at least from my side, until last week).

To summarize, we should work on a Modelica extension to handle this case in a clean way (dimensioning arrays in a Modelica model or function by reading data from file). If this is not done, then there will be vendor-specific enhancements that are not Modelica-compliant in order that important end-user requirments can be fulfilled (this feature is needed whenever models from PDE environments, like FE or CFD, shall be imported in a Modelica model).

modelica-trac-importer commented 7 years ago

Comment by otter on 13 Sep 2015 20:09 UTC I will try to fill a separate ticket before the design meeting. I need some time for it in order to analyze how this feature is currently used in different libraries.

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 14 Sep 2015 06:51 UTC I think the discussion (though still important) diverged from what the tickets was meant to be about. I.e., removing the Modelica_ prefix from the name that suggests that the library is fit to be merged into MSL and therefore also compliant with the specs. Which is currently not the case and therefore should probably simply be called LinearSystems2 or even LinearSystems.

modelica-trac-importer commented 7 years ago

Comment by otter on 14 Sep 2015 07:00 UTC Replying to [comment:7 dietmarw]:

I think the discussion (though still important) diverged from what the tickets was meant to be about. I.e., removing the Modelica_ prefix from the name that suggests that the library is fit to be merged into MSL and therefore also compliant with the specs. Which is currently not the case and therefore should probably simply be called LinearSystems2 or even LinearSystems.

Hm. Seems my respond was not clear enough, therefore more explicite: My goal is to make the Modelica_LinearSystems2 library Modelica compliant and include it in MSL (because important base functionality whenever one wants to work with controllers in a Modelica model).

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 14 Sep 2015 07:25 UTC I know what the goal is but the name as it currently is makes false promises. A more correct sequence of events would be that a library is first MLS compliant and then considered to call itself Modelica_.... This is of course also an issue for some of the other non-compliant Modelica_-libraries.

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 14 Sep 2015 09:00 UTC Replying to [comment:5 otter]:

Replying to [comment:4 stefanv]:

Getting the dimensions from a side effect is in no way similar to getting the dimensions from an input argument. If you were to allow this, then you should also allow functions that return arrays of random size.

Allowing this to be handled means that a tool must be able to change the number of equations in a model, arbitrarily, at simulation time. That does not seem like a "reasonable solution" to me.

Dymola is not doing this. Instead when translating a model the file is read and with this information the model is built. After translation of the model, the file is no longer inspected.

I did not think of this problem previously, but this feature is used also in other cases: For example, in the commercial DLR FlexibleBodies library, the information about a flexible body is read from file and this file was generated from a Finite-Element program. Again, the dimensions of vectors/matrices in a Modelica model depend on sizes of arrays on a file, and then this must also be not Modelica compliant.

I don't think there is a similar issue with FlexibleBodies library (at least I couldn't detect it); but I don't know the exact details.

The reason for the confusion is that the problem is slightly more complicated.

As indicated above the array size of an output may be given by an input formal parameters - or an expression containing them. A function call that reads a matrix from a file and returns the size is an expression containing input formal parameters - and thus standard compliant.

But using a protected variable to cache that result is not ok (I understand the underlying logic of not having the public interface depend on hidden parts, it is just that the consequences are likely not what we originally intended).

Thus the problem in:

encapsulated function fromFile 
  input String fileName="dslin.mat";
protected 
  input Integer xuy[3]=Modelica_LinearSystems2.StateSpace.Internal.readSystemDimension(fileName, matrixName);
  input Integer nx=xuy[1];
  input Integer nu=xuy[2];
  input Integer ny=xuy[3];

public 
  output StateSpace2 result(
    redeclare Real A[nx,nx],
    redeclare Real B[nx,nu],
    redeclare Real C[ny,nx],
    redeclare Real D[ny,nu]) "= model linearized at initial point";

is not that we need to read a file to determine nx, ny, nu - but that the non-input nx is used to give the size of the non-input A. (As previously indicated the "protected input" is not actually an input formal parameter.)

Thus it would be possible to remove it by using something like the following (except that it is unreadable and seems inefficient):

encapsulated function fromFile 
  input String fileName="dslin.mat";
  output StateSpace2 result(
    redeclare Real A[Modelica_LinearSystems2.StateSpace.Internal.readSystemDimension(fileName, matrixName)*{1,0,0},Modelica_LinearSystems2.StateSpace.Internal.readSystemDimension(fileName, matrixName)*{1,0,0}],
    redeclare Real B[Modelica_LinearSystems2.StateSpace.Internal.readSystemDimension(fileName, matrixName)*{1,0,0},Modelica_LinearSystems2.StateSpace.Internal.readSystemDimension(fileName, matrixName)*{0,1,0}],
    redeclare Real C[Modelica_LinearSystems2.StateSpace.Internal.readSystemDimension(fileName, matrixName)*{0,0,1},Modelica_LinearSystems2.StateSpace.Internal.readSystemDimension(fileName, matrixName)*{1,0,0}],
    redeclare Real D[Modelica_LinearSystems2.StateSpace.Internal.readSystemDimension(fileName, matrixName)*{0,0,1},Modelica_LinearSystems2.StateSpace.Internal.readSystemDimension(fileName, matrixName)*{0,1,0}]) "= model linearized at initial point";

(It might also contain some errors.)

There are also other commercial libraries in this field (especially for flexible bodies) from other organizations, and they all have the same problem (but this was not recognized, at least from my side, until last week).

To summarize, we should work on a Modelica extension to handle this case in a clean way (dimensioning arrays in a Modelica model or function by reading data from file). If this is not done, then there will be vendor-specific enhancements that are not Modelica-compliant in order that important end-user requirments can be fulfilled (this feature is needed whenever models from PDE environments, like FE or CFD, shall be imported in a Modelica model).

I also believe we should work on this. It seems the current restrictions for sizes are more formal rules than practical rules.

modelica-trac-importer commented 7 years ago

Comment by thiele on 14 Sep 2015 12:22 UTC Replying to [comment:9 dietmarw]:

I know what the goal is but the name as it currently is makes false promises. A more correct sequence of events would be that a library is first MLS compliant and then considered to call itself Modelica_.... This is of course also an issue for some of the other non-compliant Modelica_-libraries.

Well, we already indicate whether a library is deemed to be completely standard conform. It is also not uncommon that somebody thinks the library is standard conform until somebody else finds an issue (this has been the case for me...). Renaming the library at one point (maybe even renaming it again if a new, so far unknown, compatibility issue is found) doesn't help too much in my opinion.

So, if issues are found and communicated that is a first step. It is not always so easy to resolve all issues right away without sacrificing important functionality. If certain issues result in discussing improvements to the Modelica specification rather than to reduce library functionality this is not per se a bad thing in my opinion. If it turns out that there is no majority or interest for changing the specification the library will of course need to be adapted at some point.

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 20 Dec 2016 14:01 UTC Unhide for GitHub transition.

pierre-haessig commented 6 years ago

Hello,

In reference to @MartinOtter's message

To summarize, we should work on a Modelica extension to handle this case in a clean way (dimensioning arrays in a Modelica model or function by reading data from file).

is there a reference for a Modelica enhancement proposal on the possibility to define the size of an array from a external data file ? Or it has not yet been submitted?

I support the idea that defining the size of an array in an external data file is a natural extension of the present situation (allowing the size definition only withing Modelica code).

best, Pierre

tbeu commented 6 years ago

To my knowledge there is and was no other activity in MAP-Language regarding the relaxation of the size restrictions in Modelica 3.x than the above ideas from @HansOlsson, @bernhard-thiele or @svorkoetter.

beutlich commented 6 years ago

There are three options here I see:

  1. Short term: Remove the functions that depend on locally defined array dimensions.
  2. Short term: Remove the Modelica_ prefix to state that the library moved away from the Modelica standard.
  3. Long term: Standardize functions with locally defined array dimensions in the Modelica specification.

@MartinOtter What is your opinion and how to proceed here?

MartinOtter commented 6 years ago

The goal is to have a Modelica-compliant library and therefore only option (3) is useful (but it should not be "long term" but "short term"). Option (1) and (2) are not good (I completely agree to the comment here).

beutlich commented 5 years ago

@HansOlsson Should we add option (3) as short term standardization issue then?