Skip to content

Conversation

@tobolar
Copy link
Contributor

@tobolar tobolar commented Nov 9, 2023

Refs #3739

This is the next PR to #4227.
I've also performed partial regression tests of multibody examples. No difference to reference resutls indicated.

@tobolar tobolar added enhancement New feature or enhancement L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody labels Nov 9, 2023
@tobolar tobolar added this to the MSL4.1.0 milestone Nov 9, 2023
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 think it is ok, but I noticed one confusing issue:
The R_Rel in PartialForce is new - even though it was previously documented!

That breaks some of the stated compatibility issues, but on the other hand it is a clear bug.

@tobolar
Copy link
Contributor Author

tobolar commented Nov 30, 2023

That breaks some of the stated compatibility issues, but on the other hand it is a clear bug.

To me it seems straight forward to have R_rel as a counterpart to r_rel_b. So I would prefer to retain this new variable.
In which manner does it "breaks some of the stated compatibility issues"?

@HansOlsson
Copy link
Contributor

That breaks some of the stated compatibility issues, but on the other hand it is a clear bug.

To me it seems stight forward to have R_rel as a counterpart to r_rel_b. So I would prefer to retain this new variable. In which manner does it "breaks some of the stated compatibility issues"?

If it is a maintenance fix it would break the rule in Modelica.UsersGuide.ReleaseNotes.VersionManagement

Introducing a new name in the public section of a class (model, package, ...) or in any section of a partial class is not allowed. Since otherwise, a user might use this new name and when storing its model and loading it with an older bug-fix version, an error would occur.

However, one could argue that it isn't a new name as it was already documented.

@tobolar
Copy link
Contributor Author

tobolar commented Dec 1, 2023

However, one could argue that it isn't a new name as it was already documented.

Good Idea.

Yet another possibility would be to wait with merging till there is a dedicated v4.1.0 branch.

@HansOlsson HansOlsson self-requested a review December 1, 2023 10:07
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.

Looks good.

@beutlich beutlich removed their request for review January 12, 2024 21:58
@arunkumar-narasimhan
Copy link
Collaborator

@MartinOtter , could you please provide your review for this PR?

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.

  • This proposal is not backwards compatible: Assume PartialForce is used outside of MSL, then this will give an error, because the new Partial Force has R_rel, which an existing model of Partial Force might have in some (rare) cases.
  • The new implementation is significantly less efficient: Computing a relative rotation matrix requires 27 multiplications. Furthermore two forces and torques are transformed with this matrix: In total 27+3+3 = 33 multiplications
    The current implementation does not compute R_rel, but uses Frames.resolveRelative, which requires 6 multiplications. Since 2 vectors (force and torque) are transformed in the "old" PartialForce, a total of 12 multiplications is used to achieve the same result
    (12 << 33). For pure force elements (torque = 0), R_rel is not needed, and therefore efficiency is reduced for these force elements.

@casella
Copy link
Contributor

casella commented Jan 24, 2024

@tobolar, we have a cut-off date on Feb 2, if you think you can manage this issue by then, please go ahead, otherwise I'd suggest we reschedule for 4.2.0. There is no need to delay 4.1.0, which already has a lot of improvements, the next release will be within one year. Thanks!

@AHaumer
Copy link
Contributor

AHaumer commented Jan 24, 2024

@MartinOtter and @HansOlsson please review and finalize this PR, otherwise agree with @tobolar on shifting the milestone.
Feel free to invite additional reviewers.

@tobolar
Copy link
Contributor Author

tobolar commented Jan 24, 2024

  • This proposal is not backwards compatible: Assume PartialForce is used outside of MSL, then this will give an error, because the new Partial Force has R_rel, which an existing model of Partial Force might have in some (rare) cases.

Ok, this is reasonable.

  • The new implementation is significantly less efficient: Computing a relative rotation matrix requires 27 multiplications. Furthermore two forces and torques are transformed with this matrix: In total 27+3+3 = 33 multiplications
    The current implementation does not compute R_rel, but uses Frames.resolveRelative, which requires 6 multiplications. Since 2 vectors (force and torque) are transformed in the "old" PartialForce, a total of 12 multiplications is used to achieve the same result
    (12 << 33). For pure force elements (torque = 0), R_rel is not needed, and therefore efficiency is reduced for these force elements.

Hmm. I didn't consider this. Just considered the users view of understanding, where having the relative Martix R_rel doing all the transformation is straightforward.
I will revert the introduction of R_rel and fix the documentation accordingly.

@tobolar tobolar modified the milestones: MSL4.1.0, MSL4.2.0 Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or enhancement L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants