-
Notifications
You must be signed in to change notification settings - Fork 168
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 assertions #4276
base: master
Are you sure you want to change the base?
Fix assertions #4276
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.
Apart from the previous proposal to have two different tolerances (rtol and vtol) there's another issue with the test:
It is still a one-sided test!
Co-authored-by: Hans Olsson <[email protected]>
Co-authored-by: Hans Olsson <[email protected]>
@@ -3734,14 +3740,14 @@ a linear damper is connected here. | |||
v(fixed=true), | |||
n={0,1,0}, | |||
s(start=-1, fixed=true), | |||
a(fixed=true, start=-0.81)) annotation (Placement(transformation( | |||
a(fixed=true, start=9-world.g)) annotation (Placement(transformation( |
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.
@MartinOtter As I fixed the assert-clause below, it was triggered. So I changed prismatic1.a.start
to far more precise value to fulfill the assertion. If this was not originally intended in the test model, then some other estimation shall be taken. But the original start=-0.81
is too hight.
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.
I was a bit lost here. Having -0.81 as a magic number is weird, changing it to 9-world.g just seems like magic!
After some thinking and translating the model in Dymola I got that the model has the following parameters:
parameter Modelica.Units.SI.TranslationalSpringConstant spring_c=10
"Spring constant";
parameter Modelica.Units.SI.Length spring_s_unstretched=0.1
"Unstretched spring length";
parameter Modelica.Units.SI.Position prismatic_s_start=-1
"Relative distance between frame_a and frame_b";
parameter Modelica.Units.SI.Mass m=1;
Adding them where appropriate allows us to write:
a(fixed=true, start=spring_c*(-prismatic_s_start-spring_s_unstretched)/m - world.g)
I don't know if there is an intuitive explanation for the formula, but at least it seems to reduce the magic (and not only does it work, it's also unit-correct).
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.
@HansOlsson Perfect, I like it! (Why didn't I just think of that.)
The question remains - was it an intention to cause the difference for the assert? Or is that precise value just fine now?
Separate tolerances for position and velocity quantites
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.
As indicated I think that the Spring-formula now looks too much like magic.
This PR especially
fixes senseless assertions like
(so comparing the variable
body.r_0[2]
to itself).Fixing this, the assertion is triggered for tol=1e-4. So I additionally changed the intialization
prismatic1.a(..., start=9-world.g)
to have more precise value.Note: for
ModelicaTest.MultiBody.Forces.Spring
, this PR changes values of reference signalsprismatic1.s
andprismatic1.v
over time. So regression check could be broken => reference signals shall be recalculated/re-generated.No such behaviour expected for
ModelicaTest.MultiBody.Forces.Spring2
.Fixes missing absolute differences like
abs(x - y)
for assertions.Separate tolerances to different physical quantities
SI units for tolerances
Refs #4193