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

Add unit="1" to quaternions #4397

Merged
merged 2 commits into from
Jul 1, 2024
Merged

Add unit="1" to quaternions #4397

merged 2 commits into from
Jul 1, 2024

Conversation

HansOlsson
Copy link
Contributor

@HansOlsson HansOlsson commented Apr 24, 2024

Note that Modelica.Mechanics.MultiBody.Frames.Quaternions.der_Orientation already have a corresponding unit. That has two implications:

  • It is the correct unit; as it cannot be anything else. (Also clear from the constraint that q*q=1.)
  • In many cases we could already deduce the unit for the quaternion based on its derivative.

However, if we didn't compute the derivative in a specific model that deduction wouldn't happen. Additionally it seems a bit counter-intuitive to go that indirect route.

Note that Modelica.Mechanics.MultiBody.Frames.Quaternions.der_Orientation already have a corresponding unit.
That has two implications:
* It is the correct unit; as it cannot be anything else.
* In many cases we could already deduce the unit for the quaternion based on its derivative.

However, if we didn't compute in a specific model the derivative that wouldn't happen.
Additionally it seemed a bit counter-intuitive to go that indirect route.
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.

Looks excellent to me. I'm approving with the caveat that we are currently not in a position where we can evaluate the implications for unit checking, since our implementation is currently not paying much attention to arrays.

@casella casella self-requested a review June 22, 2024 16:15
Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

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

LGTM

@casella casella enabled auto-merge (squash) June 22, 2024 16:16
@HansOlsson
Copy link
Contributor Author

What should I do regarding the CLA-check?

@casella
Copy link
Contributor

casella commented Jun 29, 2024

Looks stuck. No idea why.

@casella casella merged commit 8cb2ffd into modelica:master Jul 1, 2024
1 of 2 checks passed
@Esther-Devakirubai
Copy link
Contributor

@casella Can this be backported to maint/4.1.0? No Milestone either

@casella
Copy link
Contributor

casella commented Jul 31, 2024

@casella Can this be backported to maint/4.1.0? No Milestone either

Please do.

Esther-Devakirubai pushed a commit that referenced this pull request Aug 13, 2024
Note that Modelica.Mechanics.MultiBody.Frames.Quaternions.der_Orientation already have a corresponding unit.
That has two implications:
* It is the correct unit; as it cannot be anything else.
* In many cases we could already deduce the unit for the quaternion based on its derivative.

However, if we didn't compute in a specific model the derivative that wouldn't happen.
Additionally it seemed a bit counter-intuitive to go that indirect route.
@Esther-Devakirubai
Copy link
Contributor

Backporting this to maintenance branch by #4445

@HansOlsson HansOlsson deleted the QUnit branch August 13, 2024 12:30
casella pushed a commit that referenced this pull request Aug 13, 2024
Note that Modelica.Mechanics.MultiBody.Frames.Quaternions.der_Orientation already have a corresponding unit.
That has two implications:
* It is the correct unit; as it cannot be anything else.
* In many cases we could already deduce the unit for the quaternion based on its derivative.

However, if we didn't compute in a specific model the derivative that wouldn't happen.
Additionally it seemed a bit counter-intuitive to go that indirect route.

Co-authored-by: Hans Olsson <HansOlsson@users.noreply.github.com>
@beutlich beutlich added this to the maintenance milestone Aug 13, 2024
@beutlich beutlich changed the title Add unit="1" to quaternions. Add unit="1" to quaternions Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants