Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Significant issue in MLS interpretation between SystemModeler, Dymola and Modelon Impact #2835

Closed
hubertus65 opened this issue Jan 26, 2021 · 29 comments
Labels
bug Something isn't working clarification Specification of feature is unclear, but not incorrect discussion Indicates that there's a discussion; not clear if bug, enhancement, or working as intended tool-issue Issue in tool(s) - not the specification itself worksforme
Milestone

Comments

@hubertus65
Copy link
Member

Gents,

I saw a post about a BusinessSimulation library available for free in the SystemModeler Library store, and since I was interested in that, I checked the license, extracted the pure Modelica, and tried it. Neither in Dymola nor in Modelon Impact this was accepted as legal, the most common error message being about discrete variables not being assigned in when-clauses, but also others. I didn't get one Example I tried to run.

We have worked quite hard over years to get the specification into a state that we made good progress towards "write once, run everywhere" Modelica models. This was a disappointment. There are definitely issues where there are different opinions on what is legal semantics for discrete-time models. There are other, much smaller issues, and style issues about how to do libraries as well, but they don't matter as much.

For simplicity, I extracted the pure Modelica code that is inside a wrapper for SystemModeler and attached it here.

BusinessSimulation 1.0.0.zip

I'd like to get comments from others on the legality of this, maybe SimulationX or MapleSim want to try the models?

Thanks, Hubertus

@hubertus65 hubertus65 added bug Something isn't working discussion Indicates that there's a discussion; not clear if bug, enhancement, or working as intended clarification Specification of feature is unclear, but not incorrect labels Jan 26, 2021
@HansOlsson
Copy link
Collaborator

HansOlsson commented Jan 26, 2021

Note that the units also cause problems in Dymola and is not according to https://specification.modelica.org/master/unit-expressions.html

  • "each" is not a valid unit-string. I believe this is related to unit for counting things; related to More flexible unit handling #1024 and unit for Integer types #1025. I believe that "1" should currently be used.
  • "similarly CU", "EUR" for money. For currencies we also have the problem that conversion factors vary with time - and we also have terms like "1993 Dollar".
  • "yr" (as abbreviation of "year") is not a valid unit-string. Dymola unfortunately recognizes the older variant "r" for "radian" (SI only has "rad") and thus interpret this as yocto-radian, which is a tiny angle. Note that SI does not have "year" - assumedly because there are so many slightly different variants. (Minutes, hours, and days are "sort of ok"; with day meaning 24 hours.)

Additionally BusinessSimulation.Flows.Unidirectional.Transition seems to breaks the rules regarding use of conditional components (outside of connections cp. https://specification.modelica.org/master/class-predefined-types-and-declarations.html#conditional-component-declaration ), and the ones for non-real equations.

And the restriction regarding Reals declared as discrete isn't just Dymola but specified in https://specification.modelica.org/master/class-predefined-types-and-declarations.html#component-variability-prefixes-discrete-parameter-constant (I thought there was an old issue for this, but I didn't find it.)

@eshmoylova
Copy link
Member

I tried running 10 examples that were included in the library (10 models that had the experiment annotation) in MapleSim. Two failed with lookup issues:

2: ERROR in model Examples.LookupFunctions: cannot resolve tableFunction.Smoothness.LinearSegments in modification in model BusinessSimulation.Examples.LookupFunctions; there is no Smoothness.LinearSegments in variable BusinessSimulation.Examples.LookupFunctions.tableFunction

tableFunction is a component in Example.LookupFunction, Smoothness is one of the imports in it! In a composite name if the first identifier is a component we are supposed to search among components. See the specification, second and third bullet item. That is a very serious violation of the lookup rules IMO.

7: ERROR in model Examples.SIR: while resolving type Converters.ConstantConverter.OutputType of parameter BusinessSimulation.Examples.SIR.baseReproductionNumber in model BusinessSimulation.Examples.SIR: lookup in a class that does not satisfy the requirements of a package is limited to encapsulated classes; found non-encapsulated type BusinessSimulation.Converters.ConstantConverter.OutputType inside block BusinessSimulation.Converters.ConstantConverter which violates the requirement that a package expects a class or constant for component BusinessSimulation.Converters.ConstantConverter.y

The declaration is:

  parameter Converters.ConstantConverter.OutputType baseReproductionNumber(displayUnit = "each") = 2.5 "Value of constant output (R0.value)";

As the error states Converters.ConstantConverter does not satisfy the requirement of a package and OutputType is not an encapsulated class, see the same specification part, fourth bullet item. Moreover, OutputType is a parameter not a class and cannot be used as a type of a component.

In other example, Examples.AssemblyLine and Examples.SimpleProductionChainII failed with inconsistent initial equation. The remaining examples:

Examples.LotkaVolterraEquationsRevisited
Examples.LotkaVolterraSystems
Examples.LoveHateDynamics
Examples.ManagingEmployment
Examples.SimpleProductionChain
Examples.SimpleProductionChainIII

all use at some point Converters.ConstantConverterRate which uses Converters.RateConversion and has a parameter in a modification of a constant:

block RateConversion "Converts a rate given per time base A to a rate per time base B"
  ...
  parameter TimeBases timeBaseB = TimeBases.seconds "TimeBase to convert to (default = seconds)" annotation(Evaluate = true, Dialog(group = "Structural Parameters"));
  ...
  constant String unitOutput = "1/" + BusinessSimulation.Constants.timeBaseUnits[timeBaseB] "Unit for the output";

So they all fail in MapleSim.

@perost
Copy link
Collaborator

perost commented Jan 26, 2021

All of the examples are rejected by OpenModelica too. Most of them fail because they use a component of type SourcesOrSinks.Growth, which is illegal since SourcesOfSinks is a partial package. Some models fail for other reasons already mentioned.

@maltelenz
Copy link
Collaborator

I agree that this is a problem, and has been for a long time.
My experience is that the Modelica world is slowly moving in the right direction, but maybe this is colored by the fact that I myself have been more involved with design discussions recently.

We (for System Modeler) will of course go through the cases mentioned here, and we aim to get better at rejecting all the things that are not valid according to the specification.
If we find the specification unclear, we will open specific issues to clarify as needed (some already have issues, like the units mentioned above).
This is something we have been working hard on recently, and we intend to continue improving in coming versions.

A vital part of rejecting invalid things, is a good suite of test models, and we have found the ModelicaCompliance library to be very helpful in this regard.
There have been some minor updates to it, but it is lagging far behind in covering the many changes, improvements and clarifications made in the language.
Having an updated ModelicaCompliance library is in my opinion a very important avenue to give all tool vendors better tools to strive towards compatibility.

@hubertus65
Copy link
Member Author

Thanks all for looking into this quickly and thoroughly. I had noted some of the issues Hans noted as well but didn't report. It would be nice to get the library into a specification-compliant form. I found the library interesting since it is from a new domain, but don't currently have the bandwidth to help with that.

@gwr69
Copy link
Contributor

gwr69 commented Jan 28, 2021

Dear all,

I must "out" myself as the unfortunate author of that ambitious, unphysical library (the first I have ever written for that). Like Icaraus in ancient times I must realize: He (or she) who has high-flying ambitions, can fail most spectactularly. Given the great clarity of the Modelica specification—how could this fail on such a grand scale? :-)

Unfortunately, coming from a different domain makes the use of Modelica not an easy one and naively I had expected that using 300+ unit tests written in Mathematica using the Wolfram Language with its VerificationTest function nicely applicable to models written in Modelica for System Modeler should have avoided all of this. As you can at least see from the videos taken, the user experience in SystemModeler is a rather nice one—I have the sincere hope of making it at least partially available for users of other platforms as well (after all there must be some business case for proprietary tools?).

Allow me to quickly comment on some issues raised here simply as being a programmer very interested in complying to a precisely and unambiguously specified modeling language to the best of his abilities:

  1. I have indeed defined a new type called "Amount" and honestly nobody in business will be happy being condemned to just use "1" for counting because people in physics demand it. After all, we can define units and "each" is a string — not a symbol violating reserved words? There can even be custom conversion tables so given that a chemist has "mole" why can't we have "each", "pair", "thousand" etc.?

  2. There is complaint about "years" being abbreviated as "yr" and while an analogous case as in (1.) might be made, the abbreviation "yr" is at least as common to find (even recommended in some sciences) as "y". Note btw that the official recommendation for non-SI units in the case of "years" is actually "a" from Latin "annus".

  3. Regarding money I am a bit confused. I would see the absence of a conversion factor (indeed they are time-dependent) as to be taken as "no need for conversion" so that the base conversion factor is to be "1". One may argue that there should be different quantities (leading to maybe an extended philosophical discussion):

type Money = Real(quantity = "Money", unit = "CU"); 
type Money_USD = Real(quantity = "Money_USD", unit = "USD");
type Money_EUR = Real(quantity = "Money_EUR", unit = "EUR");
  1. (continued) Is this the remedy most of you would recommend? We might probably agree upon the fact, that money should not simply have unit = "1" as it carries a value in the same way an electron carries its charge?

  2. Thanks for catching partial package SourceOrSinks (that happens when you use a mouse and a drop down menu for creating a new class)—which is the easiest to correct. :)

  3. The violations regarding lookup rules and the use of OutputType call for a closer look...

Thanks to you all for pointing out my follies.

I will probably publish the library on my public GitHub account as well and hope that—as is common practice—errors (euphemistically called "bugs" these days) will be raised as issues there.
I will try to come up with a fixed version v1.0.1 shortly that will hopefully not have most of these issues. BTW having working instances of other platforms available as library developer would be a great benefit (wink, wink). ;-)

@henrikt-ma
Copy link
Collaborator

As you can at least see from the videos taken, the user experience in SystemModeler is a rather nice one—I have the sincere hope of making it at least partially available for users of other platforms as well (after all there must be some business case for proprietary tools?).

Yes, of course every tool vendor needs to have a good business case, but I can guarantee that Wolfram does not want to make a business case of using Modelica in a way that is not compliant with the specification. One of the great user benefits of choosing a Modelica-based tool for his/her modeling work is the access to a free and tool-independent language specification. This, I assume is an important business case for any Modelica tool vendor in a world with many competing modeling technologies.

@gwr69
Copy link
Contributor

gwr69 commented Jan 28, 2021

And the restriction regarding Reals declared as discrete isn't just Dymola but specified in https://specification.modelica.org/master/class-predefined-types-and-declarations.html#component-variability-prefixes-discrete-parameter-constant (I thought there was an old issue for this, but I didn't find it.)

Can someone help me out here (maybe point me to a separate issue or to clearly written sections in the specs)? In what is called ConstantConverter I am simply changing a parameter into a continuous time signal. Compare the equivalent block in the SystemDynamics library:

block Const "A constant factor"
  parameter Real k = 0 "Constant additive term";
  Modelica.Blocks.Interfaces.RealOutput y "Output variable";
equation
  y = k;
end Const;

That code does not raise any issue, but effectively y is discrete in the sense that it is changed only at initialization and has der(y) = 0 during the whole simulation. Anyhow, if one were to give y the prefix discrete then to not get a warning for pre(y) being initialized using start values, the model would need to look like this:

block Const "A constant factor"
 parameter Real k = 0 "Constant additive term";
 discrete Modelica.Blocks.Interfaces.RealOutput y "Output variable";
initial equation
   pre(y) = k; // making initialization from start value explicit
equation
 when initial() then
      y = k;
  end when;  
end Const;

That code begs two questions: a) is there a better way than this (rather bloated) example to do this and b) is there in fact no good reason to have y be a discrete variable in the first place?

UPDATE
I am only having access to OpenModelica next to System Modeler and OpenModelica never complains about an equation involving a discrete Real variable outside of a when-equation ...

@henrikt-ma
Copy link
Collaborator

And the restriction regarding Reals declared as discrete isn't just Dymola but specified in https://specification.modelica.org/master/class-predefined-types-and-declarations.html#component-variability-prefixes-discrete-parameter-constant (I thought there was an old issue for this, but I didn't find it.)

Yes, I also expected there to be an issue on the matter, but I couldn't find it either.

b) is there in fact no good reason to have y be a discrete variable in the first place?

Only for the clarity it brings in form of a guarantee that the variable is discrete-time. The issue that also I couldn't find was asking why the discrete prefix couldn't be an actual variability prefix, with assignments in when-clauses as one of the ways one can comply with the declared variability. (Another way being to solve for the variable in an equation where everything else has discrete-time or lower variability.)

@henrikt-ma
Copy link
Collaborator

is there a better way than this (rather bloated) example to do this

Since the output has parameter variability, why not use the parameter prefix?

You can read more on this topic here: modelica/ModelicaStandardLibrary#3112

@henrikt-ma
Copy link
Collaborator

I think this is the start of the discussion @HansOlsson and I were looking for: #2521 (comment)

I also found a fragment in the same spirit here: #2526 (comment)

@HansOlsson
Copy link
Collaborator

That code begs two questions: a) is there a better way than this (rather bloated) example to do this and b) is there in fact no good reason to have y be a discrete variable in the first place?

b. There's no good reason to have it be discrete.
That goes together with synchronous Modelica where variables are implicitly clocked.

The benefit is that a block that adds, subtracts, multiplies etc two signals doesn't need to care whether the signals are continuous, discrete, or clocked; so we only need one for all of those cases.

@henrikt-ma
Copy link
Collaborator

The benefit is that a block that adds, subtracts, multiplies etc two signals doesn't need to care whether the signals are continuous, discrete, or clocked; so we only need one for all of those cases.

This would be better handled by allowing functions to be used as blocks: The fact that a function doesn't hold a state is both an important characteristic often emphasized in block-oriented control systems work, and would for Modelica be the important property that could allow our variability analysis do its job.

@gwr69
Copy link
Contributor

gwr69 commented Jan 29, 2021

Since the output has parameter variability, why not use the parameter prefix?

[I have not read through the cited ticket, yet.]
I am a bit confused because in 9.3 of the Specification v3.5 I find:

A connector component shall not be declared with the prefix parameter or constant. In the connect-equation the primitive components may only connect parameter variables to parameter variables and constant variables to constant variables.

To me that simply leaves discrete or no prefix. Am I missing something?

@gwr69
Copy link
Contributor

gwr69 commented Jan 29, 2021

That code begs two questions: a) is there a better way than this (rather bloated) example to do this and b) is there in fact no good reason to have y be a discrete variable in the first place?

b. There's no good reason to have it be discrete.
That goes together with synchronous Modelica where variables are implicitly clocked.

The benefit is that a block that adds, subtracts, multiplies etc two signals doesn't need to care whether the signals are continuous, discrete, or clocked; so we only need one for all of those cases.

So in my simple case none of the advantages given in 4.4.4 of the Specification v3.5—see below—does apply?

[A discrete-time variable is a piecewise constant signal which changes its values only at event instants during simulation. Such types of variables are needed in order that special algorithms, such as the algorithm of Pantelides for index reduction, can be applied (it must be known that the time derivative of these variables is identical to zero). Furthermore, memory requirements can be reduced in the simulation environment, if it is known that a component can only change at event instants.

@HansOlsson
Copy link
Collaborator

HansOlsson commented Jan 29, 2021

That code begs two questions: a) is there a better way than this (rather bloated) example to do this and b) is there in fact no good reason to have y be a discrete variable in the first place?

b. There's no good reason to have it be discrete.
That goes together with synchronous Modelica where variables are implicitly clocked.
The benefit is that a block that adds, subtracts, multiplies etc two signals doesn't need to care whether the signals are continuous, discrete, or clocked; so we only need one for all of those cases.

So in my simple case none of the advantages given in 4.4.4 of the Specification v3.5—see below—does apply?

[A discrete-time variable is a piecewise constant signal which changes its values only at event instants during simulation. Such types of variables are needed in order that special algorithms, such as the algorithm of Pantelides for index reduction, can be applied (it must be known that the time derivative of these variables is identical to zero). Furthermore, memory requirements can be reduced in the simulation environment, if it is known that a component can only change at event instants.

We should remove those statements. (Added: As many other problematic statements they are old. In this case they originate in Modelica Specification 1.1.)

The first part is actually false, and should be removed. If the variable is discrete then index-reduction cannot assume that the derivative is zero as that breaks index-reduction. A variable (such as a parameter) with zero derivative the entire integration can be handled specially, but if the variable jumps differentiation just breaks down.

The second part is sort of true (might actually implement it some day) - but you don't need to declare the variable as discrete to do that. A tool should be able to deduce that property directly.

@gwr69
Copy link
Contributor

gwr69 commented Jan 29, 2021

While we are checking compatibility across all relevant tools, I would like to point at something very basic: Design. Could maybe the other platforms report on how they are rendering Icons and Diagrams?

In the following examples I have placed images taken from System Modeler on the left hand side and images taken from Open Modelica v1.16 on the right hand side. I find these differences striking—how can we come up with complicated language compatibility when drawing lines, points, and arrows are not even matching up? ;-)

Class Browser

image

SourcesOrSinks Package

image

Icons

image

Diagrams

image

@henrikt-ma
Copy link
Collaborator

Since the output has parameter variability, why not use the parameter prefix?

[I have not read through the cited ticket, yet.]
I am a bit confused because in 9.3 of the Specification v3.5 I find:

A connector component shall not be declared with the prefix parameter or constant. In the connect-equation the primitive components may only connect parameter variables to parameter variables and constant variables to constant variables.

To me that simply leaves discrete or no prefix. Am I missing something?

You are right, one needs to rely on tools doing some bookkeeping to be able to take advantage of the constrained variability of the block's output. Hence, if the output was set equal to discrete-time variable assigned in a when-clause, the source variable could be declared discrete just for clarity, and you could hope that the same sort of bookkeeping that kicks in for parameters and constants would also keep track of the block's output being discrete-time (possibly allowing some downstream computation to only run at events instead of all the time).

In your case, however, it looks as the variability you want to capture is parameter, and then you should be able to just mimic the way it's done in Modelica.Blocks.Sources.Constant.

(I must admit that I don't quite understand the reasons behind not allowing variability prefixes on connector components as a way of expressing upper bounds on the variability of the variables of the connect set. To me, this would seem useful for blocks as well as functions. However, I can see that it would be a hassle to support all needed combinations in a library such as Modelica.Blocks as long as it is not possible to use functions instead of blocks for stateless computations – and maybe this is exactly the reason for not allowing variability to be explicitly declared…)

@gwr69
Copy link
Contributor

gwr69 commented Jan 29, 2021

In your case, however, it looks as the variability you want to capture is parameter, and then you should be able to just mimic the way it's done in Modelica.Blocks.Sources.Constant.

That is exactly the fix I have now applied. ;-)

@gwr69
Copy link
Contributor

gwr69 commented Feb 2, 2021

On short notice: I have just now released BSL v1.0.1, that should fix a lot of the issues addressed here (→Release Note v1.0.1). In OMEdit v1.16.2 all the Examples now pass the Check Model test. ( ;-) )

PS: I would still be interested to know, how the Icons and Diagrams turn out in other tools ... It's sad to see such divergence as between SystemModeler and OMEdit.

@maltelenz
Copy link
Collaborator

I would also be interested to see how the icons and diagrams look in other tools. Differences like this are a problem when designing icons in MSL as well, and if we could find out if there are parts of the specification that need clarification, that would be great.

@HansOlsson HansOlsson added this to the Phone2021-1 milestone Feb 12, 2021
@HansOlsson HansOlsson modified the milestones: Phone2021-1, Phone2021-2 Mar 15, 2021
HansOlsson added a commit to HansOlsson/ModelicaSpecification that referenced this issue Mar 15, 2021
@DagBruck
Copy link
Collaborator

Hans Olsson asked me to check in Dymola, and it is a mixed bag. Dymola can present icons in two sizes in the class browser. The bigger "icon view" which I think is closer to SystemModeler looks quite similar.

image

For the "tree view" which has very small icons the results are not so good, indicating some problem with line width scaling. This is something we need to investigate on our side.

image

I checked the other examples too with very similar results.

HansOlsson added a commit that referenced this issue Mar 19, 2021
* Clarify discrete and remove odd statement about Pantelides.
Resolves parts of #2835
Co-authored-by: Henrik Tidefelt <[email protected]>
@HansOlsson
Copy link
Collaborator

Seems that further tool investigation is needed.

@DagBruck
Copy link
Collaborator

Seems that further tool investigation is needed.

I think the bottom line is basically that SystemModeler gets it right. At least for these icons.

@gwr69
Copy link
Contributor

gwr69 commented Jun 17, 2021

I have now published the Business Simulation library (v1.0.1) in a public repository as announced:

https://github.com/bslMS/BusinessSimulation

Cheers,
Guido

@henrikt-ma
Copy link
Collaborator

henrikt-ma commented Jun 17, 2021

As far as I can tell, the examples are still not expected to work due to variability errors here: https://github.com/bslMS/BusinessSimulation/blob/19a34c8e80613ef814e6c24552061e327ce5e9c1/BusinessSimulation/Interfaces/PartialFlows/BidirectionalFlow.mo#L22

Hence, it might still be a little early to try this with tools that don't like non-discrete-time Boolean variables.

@HansOlsson
Copy link
Collaborator

Hans Olsson asked me to check in Dymola, and it is a mixed bag. Dymola can present icons in two sizes in the class browser. The bigger "icon view" which I think is closer to SystemModeler looks quite similar.

Sneakily someone (maybe @DagBruck ) corrected this for Dymola 2022x (or possibly earlier).

@HansOlsson
Copy link
Collaborator

Regarding 'year': one definition of year ('a' for annus) is the astronomical definition of 31,557,600 seconds (365.25 days - lead-seconds and the special leap-handling for some centuries not included).

@HansOlsson HansOlsson added worksforme tool-issue Issue in tool(s) - not the specification itself labels Sep 2, 2022
@HansOlsson
Copy link
Collaborator

To me it seems that the remaining issues are issues in the models and tool-issues.
Clearly it is good that all tools show the same icons for this - but it is not a specification issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working clarification Specification of feature is unclear, but not incorrect discussion Indicates that there's a discussion; not clear if bug, enhancement, or working as intended tool-issue Issue in tool(s) - not the specification itself worksforme
Projects
None yet
Development

No branches or pull requests

8 participants