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

Declare tolerance as MSL constant #4277

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

Conversation

tobolar
Copy link
Contributor

@tobolar tobolar commented Jan 22, 2024

Close #4193

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

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from 1e-4 to Modelica.Constants.eps is massive. I made a small test with System Modeler, and to my big surprise the asserts were not triggered, despite the new tolerance being extremely tight. I hope that someone with a deeper understanding of the dynamics here can approve the change to such tight tolerances.

What about all the other test models in ModelicaTest.MultiBody that still use 1e-4?

@tobolar
Copy link
Contributor Author

tobolar commented Jan 22, 2024

The change from 1e-4 to Modelica.Constants.eps is massive. I made a small test with System Modeler, and to my big surprise the asserts were not triggered, despite the new tolerance being extremely tight.

This is how I understand the examples:

  1. Elements having no number in names (e.g. damper or prismatic) and prismatic.stateSelect=StateSelect.never.
  2. Elements with "index" 1 (e.g. damper1). Damper has activated heat connector and prismatic.stateSelect=StateSelect.always.
  3. Elements with "index" 2 (e.g. damper2). Here, prismatic2 has fixed initial acceleration <> 0 and damper2 has set only initial value of the damping parameter d (i.e. damper2(d(fixed=false, start=3))). So the tool has to calculate the value of d based on the parameters and initial conditions.

Basically, all three groups have parameters and initial contitions set in a way to deliver numercally identical results - which is the case for Dymola.
So I'm happy to hear System Modeler performs the same way.


I hope that someone with a deeper understanding of the dynamics here can approve the change to such tight tolerances.

I hope so, too.

What about all the other test models in ModelicaTest.MultiBody that still use 1e-4?

Hmm, they are quite a lot. :-(
I can try to look over the next days/weeks.

Copy link
Member

@MartinOtter MartinOtter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am strongly against this change.
Several different modeling ways are compared in a simulation. Signals are computed in the order of the relative tolerance, so the correct way would be something like tol = 10*relativeTolerance. However, this cannot be expressed in Modelica. 1e-4 is far good enough as an approximation

tobolar added a commit to tobolar/ModelicaStandardLibrary that referenced this pull request Jan 24, 2024
@tobolar
Copy link
Contributor Author

tobolar commented Jan 24, 2024

Several different modeling ways are compared in a simulation. Signals are computed in the order of the relative tolerance, so the correct way would be something like tol = 10*relativeTolerance. However, this cannot be expressed in Modelica. 1e-4 is far good enough as an approximation

Ok. The tolerance is set again to 1e-4.

I will change the tolerances back to 1e-4 also in #4276

tobolar added a commit to tobolar/ModelicaStandardLibrary that referenced this pull request Jan 24, 2024
Copy link
Contributor

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are now lots of unrelated changes in graphic coordinates. Please remove.

@tobolar
Copy link
Contributor Author

tobolar commented Jan 24, 2024

There are now lots of unrelated changes in graphic coordinates. Please remove.

Sorry for this. Fixed 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.

I'm not sure this is a good idea.
I understand that we want to be less strict with unit-checking for such constants, and it matches that. (However, it is not decided yet so no need to rush.)

But if the test fails it does make sense to experiment with changing the tolerance, and thus having a constant seems problematic. (Except that the assert test is completely broken.)

Thus to me it would make sense to instead have two tolerances in the model: rtol (position) and vtol (velocity), even if they have the same value.
This is similar to the idea in #4051 to have different sources.

@henrikt-ma
Copy link
Contributor

Thus to me it would make sense to instead have two tolerances in the model: rtol (position) and vtol (velocity), even if they have the same value.

Sounds like a good alternative.

@tobolar
Copy link
Contributor Author

tobolar commented Jan 26, 2024

Thus to me it would make sense to instead have two tolerances in the model: rtol (position) and vtol (velocity), even if they have the same value.

I can change this, retaining the same values. Reasonable also to give them units (position, velocity)?

@MartinOtter Do you agree?

As this changes the original idea of this PR, I guess we can close this PR without merging.
The introduction of rtol, vtol shall IMO be done after #4276 (which fixes the assert-clauses in general) or as a part of it.

@tobolar tobolar closed this Jan 26, 2024
@tobolar tobolar reopened this Jan 26, 2024
@henrikt-ma
Copy link
Contributor

I can change this, retaining the same values. Reasonable also to give them units (position, velocity)?

Yes, this will ensure unit-awareness when viewing/modifying the parameters in context where units determined by inference are not available.

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.

Inconsistent use of tol in ModelicaTest.MultiBody.Forces.Damper
4 participants