-
Notifications
You must be signed in to change notification settings - Fork 166
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
Fix interpretation of coefficients of complex transfer function #3733
Fix interpretation of coefficients of complex transfer function #3733
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again we have the problem that the bugfix is not backwards compatible.
I'd suggest to:
- leave the old implementatio unchanged and mark it as obsolete
- move Christian's implemenatition to a new block
- explain the situation in the documentation
Also here my comment. If it is a bugfix then something was calculated wrongly before and need to be fixed. It would be very strange that we "fix" bugs by introducing new models and keep the buggy models around. That is if it is a clear bug and not just a different interpretation of the implementation. |
I agree with @dietmarw here. Bugfixes shall be fixed in place and documented in the revision annotation. I also doubt that anyone used such a function if it was plainly wrong. |
OK. The block will not be renamed or marked as obsolete: The bug fix will be implemented and documented. |
@beutlich Why is it asking again for signing the CLA? I just signed it at #3742 (comment) |
Because you (accidently ?) committed as |
Thanks for the hint. This happened accidentally. For some reason my git |
fe2c9b6
to
08c88c9
Compare
Done. I also resolved the merge conflict of #3677 and HTML tag error when rebasing. Please check again. |
08c88c9
to
ab5b5fb
Compare
@beutlich Thanks for fixing the merge conflict. On my opinion the performed changes look all good. |
parameter Real d=1/sqrt(2) "Damping coefficient"; | ||
parameter Real b[:]={1} "Numerator polynomial coefficients of the transfer function"; | ||
parameter Real a[:]={1,2*d,1} "Denominator polynomial coefficients of the transfer function"; | ||
parameter Real d=0.01 "Damping coefficient in kg/s (not the damping ratio)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it als work if physical units are used actually instead of type Real?
@HansOlsson Is it legal Modelica code to use different units in the elements of vector parameter Real d(unit = "kg/s")=0.01 "Damping coefficient in kg/s (not the damping ratio)";
parameter Modelica.Units.SI.Mass m=0.2 "Mass in kg";
parameter Modelica.Units.SI.TranslationalSpringConstant c=0.1 "Stiffness in N/m";
parameter Real b[:]={-m} "Numerator polynomial coefficients {-m} of the transfer function";
parameter Real a[:]={m,d,c} "Denominator polynomial coefficients {m,d,c} of the transfer function"; It, however, does check and simulate OK in Dymola. |
Yes. Whether it is good style is another issue.
The specification does not make it illegal, and you could even explicitly create vectors with different unit elements in Modelica like: |
While the specification doesn't make it illegal today, we also know that the specification is rather incomplete when it comes to defining a system for unit handling that would give tools the necessary foundation to reject models with unclear/erroneous unit consistency. Hence, I think one should at least be prepared that arrays with heterogenous unit could be something we'll be forced to drop support for once we take on unit checking more seriously. If one really wants to do transfer functions with units in a future-proof way, I have no better idea than making a fixed order wrapper model with only scalar coefficients that takes care of all the units, with the actual dynamics internally implemented using a unit-less transfer function. |
@beutlich I suggest to keep the implementation with |
A third option to unitless parameters or array with heterogenous unit would be something like: parameter Real d(unit = "kg/s")=0.01 "Damping coefficient in kg/s (not the damping ratio)";
parameter Modelica.Units.SI.Mass m=0.2 "Mass in kg";
parameter Modelica.Units.SI.TranslationalSpringConstant c=0.1 "Stiffness in N/m";
final parameter oneReciprocalDamping(unit="s/kg") = 1;
final parameter oneReciprocalMass(unit="kg-1") = 1;
final parameter oneReciprocalSpringConstant(unit="m/N") = 1;
parameter Real a[:]={m*oneReciprocalMass, d*oneReciprocalDamping, c*oneReciprocalSpringConstant}
"Unitless denominator polynomial coefficients {m,d,c} of the transfer function"; I think this is the cleanest Modelica can offer for now. |
Looks like a splendid place to use |
Right, here's a nicer proposal, inspired from ModelicaStandardLibrary/Modelica/Electrical/Analog/Examples/Utilities/SwitchedCapacitor.mo Lines 29 to 30 in 39fe805
parameter Modelica.Units.SI.Mass m = 0.2 "Mass in kg";
parameter Modelica.Units.SI.TranslationalDampingConstant d = 0.01 "Damping coefficient in kg/s (not the damping ratio)";
parameter Modelica.Units.SI.TranslationalSpringConstant c = 0.1 "Stiffness in N/m";
protected
constant Modelica.Units.SI.Mass oneKilogram = 1
"Helping constant to satisfy unit check";
constant Modelica.Units.SI.TranslationalDampingConstant oneUnitDampingConstant = 1
"Helping constant to satisfy unit check";
constant Modelica.Units.SI.TranslationalSpringConstant oneUnitSpringConstant = 1
"Helping constant to satisfy unit check";
public
final parameter Real a[:]={m/oneKilogram, d/oneUnitDampingConstant, c/oneUnitSpringConstant}
"Unitless denominator polynomial coefficients {m,d,c} of the transfer function"; |
Yes, this is nicer. Still, in principle, these constants should also be |
OK, I will implement @beutlich's proposal then. |
Co-authored-by: Thomas Beutlich <modelica@tbeu.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also update the table of critical errors in Modelica.UsersGuide.ReleaseNotes.Version_X_Y_0. Thanks.
Refs #3651