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.

Sign up for GitHub

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

Jump to bottom

Inconsistent use of tol in ModelicaTest.MultiBody.Forces.Damper #4193

Open
qlambert-pro opened this issue Sep 18, 2023 · 4 comments · May be fixed by #4277
Open

Inconsistent use of tol in ModelicaTest.MultiBody.Forces.Damper #4193

qlambert-pro opened this issue Sep 18, 2023 · 4 comments · May be fixed by #4277
Assignees
@tobolar
Labels
L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined

Comments

@qlambert-pro
Copy link
Contributor

The variable used as tolerance margin to decide whether two Real values are equal is used to compare position and speed at the same time.

We could turn tol into a constant as we don't expect the infer the unit of constants based on their usage or maybe use Modelica.Constants.eps?
@qlambert-pro
Copy link
Contributor Author

qlambert-pro commented Sep 18, 2023 •
edited
Loading

Same issue in ModelicaTest.MultiBody.Forces.Damper2, ModelicaTest.MultiBody.Forces.Spring and ModelicaTest.MultiBody.Forces.Spring2

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

tobolar commented Jan 22, 2024

I don't understand the point of the issue. Can you explain in more detail?

Regarding ModelicaTest.MultiBody.Forces.Spring and ModelicaTest.MultiBody.Forces.Spring2, there is a mess in using toland assert. I will prepare a fix.

@henrikt-ma
Copy link
Contributor

This has to do with unit checking. When unit inference is applied, the tol should get an inferred unit which is consistent with how it is used, but this gets problematic if it is used inconsistently unit-wise. What @qlambert-pro said about using a constant instead of a parameter has to do with the current idea that unit inference in Modelica should not be allowed for constants, so that a constant with empty unit will behave just as a literal number.

@tobolar tobolar mentioned this issue Jan 22, 2024
Open
@tobolar
Copy link
Contributor

tobolar commented Jan 22, 2024

@henrikt-ma Many thanks. I didn't fully follow the unit discussion.

Then I guess both suggestions (changing to constant and equal to Modelica.Constants.eps) are fine.
(At least Dymola didn't indicated any issue.)

@tobolar tobolar linked a pull request Jan 22, 2024 that will close this issue
Open
@qlambert-pro qlambert-pro mentioned this issue Apr 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tobolar tobolar

Labels
L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Declare tolerance as MSL constant tobolar/ModelicaStandardLibrary
4 participants
@qlambert-pro @tobolar @beutlich @henrikt-ma