#2566 closed defect (fixed)
Parameters with fixed = false and binding equation should be reported
Reported by: | Francesco Casella | Owned by: | Adrian Pop |
---|---|---|---|
Priority: | high | Milestone: | 1.9.1 |
Component: | Frontend | Version: | trunk |
Keywords: | Cc: | Jens Frenkel, Lennart Ochel |
Description (last modified by )
From the Modelica spec. 3.2 rev2, sect 8.6
In the case a parameter has both a binding equation and fixed = false, a diagnostics is recommended, but the parameter should be solved from the binding equation.
Try the following test case:
model TestParameterBinding parameter Real p(fixed = false) = 2; initial equation p = 4; end TestParameterBinding;
OMC complains about overdetermined initialization, but then gives p = 4 (instead of p = 2, as the specification requires) and does not issue any diagnostic message. I would recommend that a warning is issued, such as:
Warning: the parameter p has fixed = false and a binding equation p = 2, which is probably redundant.
In older versions of Dymola, the binding equation was silently ignored in this case. It is still ignored, even though a warning is now issued. This is a possible cause of hard-to-understand errors with models that work fine in Dymola.
Change History (64)
comment:1 by , 11 years ago
Description: | modified (diff) |
---|---|
Status: | new → assigned |
comment:2 by , 11 years ago
Status: | assigned → accepted |
---|
comment:3 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
follow-up: 6 comment:4 by , 11 years ago
The following model Fails due to my last commit:
simulation/libraries/msl31/Modelica.Mechanics.MultiBody.Examples.Elementary.InitSpringConstant.mos
Output:
[Modelica 3.1/Mechanics/MultiBody/Forces.mo:2094:5-2094:78:writable] Warning: The parameter spring.c has fixed = false and a binding equation p = 100.0, which is probably redundant.
Is it possible that this is a model issue?
comment:5 by , 11 years ago
Also 1 AVM model fails (5 in total but 4 is just additional text changes).
comment:6 by , 11 years ago
Replying to lochel:
The following model Fails due to my last commit:
simulation/libraries/msl31/Modelica.Mechanics.MultiBody.Examples.Elementary.InitSpringConstant.mos
Output:
[Modelica 3.1/Mechanics/MultiBody/Forces.mo:2094:5-2094:78:writable] Warning: The parameter spring.c has fixed = false and a binding equation p = 100.0, which is probably redundant.Is it possible that this is a model issue?
I guess it is a tool issue. The InitiSpringConstant has the following declaration:
Modelica.Mechanics.MultiBody.Forces.Spring spring( s_unstretched=0.1, c(fixed=false, start=100)) annotation (Placement(transformation(origin={50,30}, extent={{-10,-10},{10,10}}, rotation=270)));
so there is no binding equation here for spring.c, nor there is one in the model Modelica.Mechanics.MultiBody.Forces.Spring. I guess this binding p = 100.0 has been (incorrectly) added by omc.
A binding to the parameter start attribute should only be added (with a warning) to parameters having fixed = true (see #2557).
Where does the p = 100.0 binding come from?
follow-up: 9 comment:7 by , 11 years ago
The name "p" should be "spring.c". That was a mistake in my commit and is already fixed. Anyway, I will see whether I can find the reason for the binding...
follow-up: 10 comment:8 by , 11 years ago
I think the reason for the binding is that the front-end added it from the start value.
In that case the binding should have BindingSource set to BINDING_FROM_START_VALUE.
So you should probably ignore bindings that have BINDING_FROM_START_VALUE in some cases.
comment:9 by , 11 years ago
Replying to lochel:
The name "p" should be "spring.c". That was a mistake in my commit and is already fixed.
Yeah, that was obvious.
Anyway, I will see whether I can find the reason for the binding...
OK, this was the actual question :)
comment:10 by , 11 years ago
Replying to adrpo:
I think the reason for the binding is that the front-end added it from the start value.
In that case the binding should have BindingSource set to BINDING_FROM_START_VALUE.
Hmm, this looks quite dangerous. This binding only makes sense in the very special case described by #2557.
So you should probably ignore bindings that have BINDING_FROM_START_VALUE in some cases.
Wouldn't it be better that the back-end adds the binding if appropriate, rather than removing it if not?
comment:11 by , 11 years ago
I agree that this is bad, but I think we cannot remove it yet.
The problem is that OMC wants all parameters to have a binding
because we don't know in the compiler which ones are structural
parameters and which are not.
This is part of my front-end changes so that we only evaluate
(replace with) what is needed (structural parameters) and not
everything as we do now.
follow-up: 14 comment:12 by , 11 years ago
Component: | Backend → Frontend |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
Ok. Seems we handle this badly in the front-end and we don't give the correct information to the back-end.
We should never add a binding from the start value if fixed = false.
If fixed=true and there is a start value we can add it but give a warning.
Is this correct?
comment:13 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | reopened → accepted |
comment:14 by , 11 years ago
Replying to adrpo:
Ok. Seems we handle this badly in the front-end and we don't give the correct information to the back-end.
We should never add a binding from the start value if fixed = false.
Yes.
If fixed=true and there is a start value we can add it but give a warning.
Is this correct?
Exactly, this is what the new 3.3 specification says, section 8.6. The fact it says 'can' and not 'should' or 'must' means we might actually not do it, but I would suggest we do so, in order to get a consistent behaviour w.r.t. Dymola.
comment:15 by , 11 years ago
Cc: | added |
---|
Hm. I think this is a back-end problem somehow:
instantiateModel give you:
parameter Real spring.c( quantity = \"TranslationalSpringConstant\", unit = \"N/m\", min = 0.0, start = 100.0, fixed = false) \"Spring constant\";
so no binding there. But the back-end reports it like one:
"[Modelica 3.1/Mechanics/MultiBody/Forces.mo:2094:5-2094:78:writable] Warning: The parameter spring.c has fixed = false and a binding equation spring.c = 100.0, which is probably redundant.
So it seems somewhere in the back-end the binding is added from the start value.
I'm adding jfrenkel as he might know more about this.
comment:16 by , 11 years ago
Gee. I flattened MSL 3.2.1 InitSpringConstant and the error is for MSL 3.1 InitSpringConstant.
Ok. back to checking with MSL 3.1.
Well, the message is normal if you consider how it looks in MSL 3.1:
Modelica.Mechanics.MultiBody.Forces.Spring spring( s_unstretched = 0.1, c(fixed = false) = 100);
So you have fixed = false and a binding but no start value!
I think in this case we should probably consider the binding as start value.
comment:17 by , 11 years ago
No, we shouldn’t. Or is there a section about it in the spec? I guess Dymola does it that way, but that is not a good reason to do it the same way.
comment:18 by , 11 years ago
btw: This is also an issue of ThermoSysPro and probably a couple of other libraries that are developed using Dymola. Anyway, we should follow the specification in a strict way.
comment:19 by , 11 years ago
We can follow the specification by giving a warning:
Parameter has (fixed=false) and binding = X but no start value. This is not conform to the Modelica Specification. Using binding = X as start value.
otherwise some models will not work.
comment:20 by , 11 years ago
But it is confirm to the Modelica specification. That are model issues and should thus be fixed within the models.
comment:21 by , 11 years ago
Ok then. But we can tell that to the user and try to continue if possible.
I really don't like drastic measures as then models that could work will not work.
Francesco, any thoughts on this?
comment:22 by , 11 years ago
The only reasonable way to interpret that if it was a parameter is an initial equation added and a warning produced.
The only reasonable way to make a normal variable fixed=false is if it is a state, right? Then adding the binding equation would be fine if it contained der(c) in this case (see below). It should probably be fine to detect this as an error (later), and the given warning is a good hint why the subsequent error will occur.
The following does not give the warning though. Wonder what it actually looks for.
model M Real r(fixed=false) = der(r)+r-1; initial equation r = 150; end M;
comment:23 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
Fixed in r19014.
The question that remains is if this:
parameter Real x(fixed = false, start = 1);
is equivalent with:
parameter Real x(fixed = false); initial equation x = 1;
If so we should probably not report a warning in this case.
comment:25 by , 11 years ago
What do we do about:
MSL 3.1, Modelica.Mechanics.MultiBody.Examples.Elementary.InitSpringConstant
?
as it now fails.
comment:26 by , 11 years ago
If it is not in compliance with the Modelic specification, we do not have to support that model. Even if it is part of MSL 3.1. Perhaps it should become a Modelica ticket, what do you think?
comment:27 by , 11 years ago
Well, the thing is that it might be compliant with Modelica Spec 3.1.
So we might need to disable some of these checks for 3.1 standard.
comment:28 by , 11 years ago
Okay. Then we do not need to create a Modelica ticket. But we do not need to support all old Modelica specifications, doesn’t we?
comment:29 by , 11 years ago
If possible, i think we should.
Give a warning if is not conform to the latest spec, but try to let it work.
follow-up: 32 comment:30 by , 11 years ago
Changed the heuristics on this bug in r19022.
- if fixed=false, no start value, and a binding -> give a warning and use the binding as start value
- if fixed=false, a start value, and a binding -> give a warning use the start value and ignore the binding
This way we get badly designed models to work and still give a warning that helps the user to fix it.
comment:32 by , 11 years ago
Replying to adrpo:
Changed the heuristics on this bug in r19022.
- if fixed=false, no start value, and a binding -> give a warning and use the binding as start value
- if fixed=false, a start value, and a binding -> give a warning use the start value and ignore the binding
This way we get badly designed models to work and still give a warning that helps the user to fix it.
Well, this is not consistent with the specification ...
comment:33 by , 11 years ago
Lennart, I understand that, but seems there are some models out there that use this pattern.
What's important is that the user gets the information and can fix their models to be spec compliant.
comment:34 by , 11 years ago
I recommend to revert the changes from r19022. I know that Dymola handles it that way and several models depend on that behavior. Anyway, it is not strict Modelica in my view. Check the following model:
model Unnamed parameter Real p(start=1, fixed=false) = 2; initial equation p=3; end Unnamed;
That is probably not the best example, but it's already late: Due to my understanding of the spec., this example should not get initialized, because it has no valid solution. So, why do we want to initialize such models?
I think we should discuss this in the next OMDev meeting. Francesco, any thoughts on this?
comment:35 by , 11 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Well, the previous example was a bit unnatural. Please check out the following (still artificial) one:
model Unnamed parameter Real p(start=1, fixed=false) = 2; Real x; initial equation x = p; equation der(x) = 0; end Unnamed;
The latest Dymola version initializes x and p with 1. Due to r19022 OpenModelica initializes x and p with 0. I strongly recommend to initialize both variables with 2, but maybe I am wrong.
Jens, can you tell us what does SimulationX?
Any comments?
follow-up: 38 comment:36 by , 11 years ago
Why do we initialize x and p with 0? We ignore the binding so we should have:
p(start = 1, fixed = false)
Am I missing something?
comment:37 by , 11 years ago
Maybe we should open a ticket about this on Modelica Trac.
I'll do that.
comment:38 by , 11 years ago
Replying to adrpo:
Why do we initialize x and p with 0? We ignore the binding so we should have:
p(start = 1, fixed = false)
Am I missing something?
The model is under-determined, because we ignore the binding of p. Hence, x gets fixed and both variables get initialized to x.start = 0.
But, that model is not under-determined.
comment:40 by , 11 years ago
So Dymola actually ignores the binding and uses the start value as the binding for p?
comment:41 by , 11 years ago
No, Dymola ignores the binding, copies the start value to x (because they are alias for the initializtion) and auto-fixed x (due to under-determined system). Hence, both get initialized with 1.
comment:42 by , 11 years ago
SimulationX seems to use the binding. Also for me the binding is higher prior than the start value. That interpretation I also implemented in the omc backend. Maybe it changed until now by someone else.
follow-up: 44 comment:43 by , 11 years ago
Maybe it is a good proposal to MA to remove fixed from the parameter attributes. Than if the parameter have a binding it is fixed and if not is is not fixed and the parameter must be calculated from the initial system. To have both fixed attribute and binding derange the users.
comment:44 by , 11 years ago
Replying to jfrenkel:
Maybe it is a good proposal to MA to remove fixed from the parameter attributes. Than if the parameter have a binding it is fixed and if not is is not fixed and the parameter must be calculated from the initial system. To have both fixed attribute and binding derange the users.
I don't agree. It is very clear how to calculate the parameters and the entire initial system. I see no reason to change the specification.
comment:45 by , 11 years ago
I know changing the spec is not trivial but from my point of view
Spec 3.2 rev2 chapter 8.3
In the case a parameter has both a binding equation and fixed = false a diagnostics is recommended, but the parameter should be solved from the binding equation.
is not very clear. Maybe I overlooked some other place in the spec (perhaps you can enlighten me) about that issue.
So what means a diagnostic is recommended.
Is there some other place in the specification about that diagnostic. It is deterministic? I don't ask as a compiler developer. I as as a modeler.
follow-up: 47 comment:46 by , 11 years ago
It means a warning (a diagnostic message) is supposed to be output because it is a stupid thing to do even though it is allowed and well specified.
comment:47 by , 11 years ago
Replying to sjoelund.se:
It means a warning (a diagnostic message) is supposed to be output because it is a stupid thing to do even though it is allowed and well specified.
Exactly.
comment:48 by , 11 years ago
We discussed this in today’s weekly OMDev meeting:
I will change the handling to what it was before r19022 and add a new “Dymola compatibility” flag to handle it like Dymola if a special flag is used.
follow-up: 51 comment:50 by , 11 years ago
Sorry guys, I don't agree with this proposal (comment:48).
I try to recap as I understand the whole issue. Sorry, it's a bit long.
All trouble stems from the behaviour Dymola had prior to Dymola 2014, which ignored the parameter binding equation when fixed = false was set. The design pattern was at the time the one Adrian found in MSL 3.1 (comment:16): you set fixed = false on the parameter (which implicitly implied, forget about the binding equation!), and then added one initial equation to rebalance the initial equation system. Maybe the binding was used as initial guess if the start attribute was not given, I'm not sure of that. I've used this pattern myself many times in the past.
Unfortunately, this behaviour has never been specified in any version of Modelica (3.1, 3.0, 2.2, 2.1): nowhere in those specifications it is stated that a parameter binding equation had to be ignored if fixed was set to false. The trouble is, the actual Modelica specification for MSL 3.1 and earlier version was the Dymola implementation, because no other tool could really handle it. This is why we had so much fuss with MSL 3.2.1/Modelica 3.2rev2, because we had to fix a number of such issues. I don't think back-fixing earlier versions of MSL to be really Modelica compliant is feasible (think of all the problems with the Multibody library, for instance).
I have an e-mail from Sven Erik Mattsson, sent on Dec 11th, 2012 (my 43rd birthday), which states: "Dear Francesco, I have now modified Dymola to behave according to the Modelica Language specification. Dymola issues a warning for fixed false parameters with bindings and then handles them as if fixed was true." In fact, this is only true if you turn the "Pedantic mode" flag on, otherwise the model Lennart proposes in comment:34 gives a warning in Dymola 2014 FD01, but then simulates and gives p = 3 (sigh!). But I guess we all agree that libraries developed in Dymola should first pass the pedantic check, before even being tried in OMC. Do we?
Jens, regarding your (comment:45), the reason why the non-normative text "In the case a parameter has both a binding equation and fixed = false, a diagnostics is recommended, but the parameter should be solved from the binding equation" was added is precisely to reaffirm that the specification never meant that binding equations should be ignored, and that if you follow that old Dymola-specific pattern you should be warned that it won't work as you might expect, because that was never really the intention.
We might introduce Dymola compatibility flags for all the dozens issues that have been fixed in Modelica 3.2 rev2/MSL 3.2.1, but I am not sure that would be wise, because then old patterns will be carried on indefinitely into Modelica 5.0, and beyond. People that (also) want to use OMC (or SimulationX, or any other tool than Dymola) should use Modelica, not a Dymola dialect. Unfortunately, before MSL 3.2.1 was issued, the MA released many versions of the MSL that were not "standard", because there was really no practical way to double check it, but now these patterns should really be discouraged for future use.
So, regarding Adrian's (comment:21), I would indeed take drastic measures, i.e., follow the specification and not the old Dymola implementation (which has never followed the spec).
Regardin Jens' proposal (comment:43), I also agree with Lennart (comment:44) that we shouldn't try to remove fixed from parameters in the spec, which is very clear on the subject. BTW, this would cause a lot of discussion, and raise further chaos in the future. IMHO, we only need to guide the users in fixing this minor, Dymola-induced, wrong design pattern.
Regarding Adrian's (comment:19), the problem is not that the start value is missing, but the fact that fixed = false necessarily implies that some initial equation has been added in order to re-balance the initialization system, and then the binding equation should obviously not be there. Again, Dymola ignored it (and it still does, for backwards compatibility, though it at least issues a warning), but this has never been in the specification. Unless we want to change this again in 3.3 rev1, God forbid!
Summing up, my humble proposal is not to introduce any Dymola compatibility flag, but rather catch all the cases of parameters with fixed = false and binding equations, and issue the appropriate diagnostic message. For example:
The parameter spring.c has fixed = false and a binding equation spring.c = 100.0.
According to the Modelica specification, the binding equation spring.c = 100.0 is not
removed from the initial system of equations, as other Modelica tools did in the past,
so you might not get the expected results. Please fix your model by removing the
parameter binding (make sure there is no default binding in the model), or by setting
fixed = true to the parameter and then by binding it to another parameter with
fixed = false and no binding equation.
I know, this is horribly long, and it also shows that MSL 3.1 is not standard (that's the sad truth, alas) if you try it there. But at least it tries to explain the situation to the end user, and suggests a way out. Hints for improvements are welcome.
comment:51 by , 11 years ago
Replying to casella:
Summing up, my humble proposal is not to introduce any Dymola compatibility flag, but rather catch all the cases of parameters with fixed = false and binding equations, and issue the appropriate diagnostic message. For example:
We already have had a flag for different language standards, +std=3.1 to follow Modelica 3.1 rules. This has been implemented not to follow 3.1 language spec, but rather 3.1 library (MSL 3.1 era should work with +std=3.1). I would suggest just using this; not many people will launch omc with +std=3.1 anyway.
comment:52 by , 11 years ago
Fair enough. But don't tell the AVM and ThermoSysPro developers, let them fix the libraries :)
comment:53 by , 11 years ago
Well... It would break MSL 3.2.1 semantics. Could even make it disallow some backwards-compatible features if we want to be nice.
Media won't work with MSL 3.1 or 3.2 in omc anyway; so those users would have to use 3.2 semantics.
comment:54 by , 11 years ago
Discussed this with Lennart. Conclusions: keep option to ignore the binding if +std=3.1, so that we keep "MSL 3.1" semantics, which is already partially supported, otherwise issue the following warning:
The parameter %s has fixed = false and a binding equation %s = %s, which is probably redundant.\n
Setting fixed = false usually means there is an additional initial equation to determine the parameter value. The binding was ignored by old Modelica tools, but this is not according to the Modelica specification. Please remove the parameter binding, or bind the parameter to another parameter with fixed = false and no binding.
The warning is a bit long, but shorter versions might give little help as how to fix the issue.
comment:56 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
follow-up: 60 comment:57 by , 11 years ago
Just a small comment: using +std=3.1
is not very good.
As if you do omc +std=3.1
and do loadModel(Model, {"3.2.1"});
the language standard will change to 3.2.
So, unfortunately I think we will need a new flag for this.
comment:58 by , 11 years ago
Maybe we can print a warning if the language standard get increased by an api call.
comment:59 by , 11 years ago
We do... Notification: Modelica language version set to 3.2 due to loading of MSL 3.2.1.
comment:60 by , 11 years ago
Replying to adrpo:
Just a small comment: using
+std=3.1
is not very good.
As if you doomc +std=3.1
and doloadModel(Model, {"3.2.1"});
the language standard will change to 3.2.
So, unfortunately I think we will need a new flag for this.
Sorry Adrian, I really do not agree. Any library that uses MSL 3.2.1 should be compliant with Modelica 3.2 rev2, period. That was the whole point of 1 1/2 years of work by the entire MA on this issue.
If the goal is to have those few more AVM and ThermoSysPro models to run, then the solution is to fix those libraries according to the spec. We can help doing that, no problem (it is actually part of Politecnico's WP in the AVM project), and further motivate the developers, if needed. We have already done it for countless other similar issues, why making an exception for this case?
comment:61 by , 11 years ago
Ok. We'll leave it like this and then people will have to fix their models.
comment:62 by , 11 years ago
For your information, this is Sven Erik Mattson's advice:
Dear Francesco,
I agree with you. Please, don't implement my old interpretation of fixed=false parameter with a binding.
Moreover, I would like to propose that if fixed = false, a parameter binding is always accepted and
that it mustn't have parameter variability to support user friendliness
- It is easier to give a binding then add an initial equation,
- You can also turn a fixed = true parameter into a fixed false parameter, which is impossible today if it has a binding. You have to introduce a new fixed 0 false parameter and bind the old parameter to it.
Best regards
Sven Erik
From: Francesco Casella francesco.casella@…
Sent: Tuesday, February 11, 2014 12:14
To: MATTSSON Sven Erik
Cc: OLSSON Hans; ELMQVIST Hilding; HENRIKSSON Dan
Subject: Re: [Dynasim #14847] Semantics of binding equations for parameters with fixed = false
Dear Sven Erik (cc: Hilding),
thank you for your prompt feedback.
You will get the behavior you desire if you set
Hidden.OldFixedFalseParameterSemantics = false
We have the issue of being backwards compatible.
Having Hidden.OldFixedFalseParameterSemantics = true
Gives the old Dymola behavior, which exploits the binding of a fixed
false parameter
as if it was a binding to its start.
We had a long discussion among the OpenModelica developers whether we
should support this old Dymola semantics. My position was against it, as
this has never been in the Modelica specification, and if we do this,
library developers will never fix their code to be compliant with the
language specification, which is our goal since MSL 3.2.1/Modelica
3.2rev2. Maybe it was not wise not to introduce this Dymola behaviour in
the spec, but that is now a fairly old decision (see
https://trac.modelica.org/Modelica/ticket/166) and it would be weird to
revert it now.
My recommendation to library developers is to fix their older libraries,
originally developed with Dymola, and try not to rely on this semantics,
which I understand is accepted by Dymola only for backwards
compatibility, but is not recommended.
I also noticed that the model I sent you gives an error if the
"pedantic" flag is turned on, which further confirms my recommendation.
What do you think?
Best regards,
Francesco
comment:63 by , 11 years ago
Note that this pattern also appears in ThermoPower for 2 models that do not work anymore:
https://test.openmodelica.org/libraries/ThermoPower/files/ThermoPower.Test.GasComponents.TestGasTurbineStodola.err
https://test.openmodelica.org/libraries/ThermoPower/files/ThermoPower.Test.GasComponents.TestGasMixer.err
comment:64 by , 11 years ago
We have a webpage about compatibility issues: WritingCompliantLibraries.
Francesco, can you add a small text about this issue and give some hint on how people can fix their models? We could also add something about fully specified initial conditions.
I wonder if it wouldn't be better to actually give a link to this page in OMC warnings and errors in some cases.
Fixed in r18982.
I added the recommended warning:
The model from above model gets not initialized anymore, because it has no valid solution.