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

Fix assertions #4276

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Fix assertions #4276

wants to merge 13 commits into from

Conversation

tobolar
Copy link
Contributor

@tobolar tobolar commented Jan 22, 2024

This PR especially

  1. fixes senseless assertions like

    assert( body.r_0[2] - body.r_0[2] < tol ...);
    

    (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 signals prismatic1.s and prismatic1.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.

  2. Fixes missing absolute differences like abs(x - y) for assertions.

  3. Separate tolerances to different physical quantities

  4. SI units for tolerances

Refs #4193

@tobolar tobolar added the L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined label Jan 22, 2024
Copy link
Contributor

@HansOlsson HansOlsson left a 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!

@beutlich beutlich removed their request for review January 24, 2024 21:51
@@ -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(
Copy link
Contributor Author

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.

Copy link
Contributor

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).

Copy link
Contributor Author

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?

Copy link
Contributor

@HansOlsson HansOlsson left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants