-
Notifications
You must be signed in to change notification settings - Fork 167
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!
ModelicaTest/MultiBody.mo
Outdated
@@ -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?
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 think the actual value is just fine now - but I would prefer replacing the magic 9 by a non-magic formula as described above. I don't think the assert was supposed to trigger.
I believe the history of the model went something like this:
- Original model with code added in some order:
- Originally
g
was 9.81 (something like that was common when I was in school, I think we used 9.82 in Lund - but Munich is south of here). - Based on that value for
g
and the spring as given above the start-value was given as9-g
and evaluated, where 9 corresponds tospring_c*(-prismatic_s_start-spring_s_unstretched)/m
. - A non-working assert was added, it didn't trigger so it was assumed to work!
- Originally
- Then
g
was replaced by the official 9.8065, but since the assert was broken it wasn't detected. - And here the assert is being corrected.
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 have adapted your suggestion introducing a final parameter for s.start
(prismatic_s_start
itself is not a parameter) in b118bb6.
P.S. To me g
being 9.82 sounds like from another universe. ^^
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.
724a20c
to
b118bb6
Compare
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.
Looking good
parameter SI.Position rtol=1e-4; | ||
parameter SI.Velocity vtol=1e-4; | ||
|
||
final parameter SI.Position s_start = -1 "Initial relative distance between world frame and body"; |
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.
May I ask why the parameter is made final, but not making the modifications using it final?
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.
No particular reason for this. I just didn't touch the modifiers. The parameter itself is final just to retain its value.
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