Skip to content

REFACTOR: Refactored quaternion implementation#6151

Merged
Alberto-DM merged 31 commits into
mainfrom
Fix_coordinate_system_mode_change
May 15, 2025
Merged

REFACTOR: Refactored quaternion implementation#6151
Alberto-DM merged 31 commits into
mainfrom
Fix_coordinate_system_mode_change

Conversation

@Alberto-DM

Copy link
Copy Markdown
Contributor

Description

The quaternion implementation used for the coordinate systems rotation was incomplete and unable to handle special cases.
Now the quaternions have their own module, named Quaternion with the relevant algebra and operations defined.
As an added bonus, all possible Euler rotations are now supported, not just ZXZ and ZYZ.
A MathUtils module has been also created to contain some mathematical utility functions.
Extensive unit tests have been also added.

Issue linked

Address the issue #6037

@ansys-reviewer-bot

Copy link
Copy Markdown
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@github-actions github-actions Bot added the bug Something isn't working label May 13, 2025
@Alberto-DM Alberto-DM changed the title REFACTOR: Quaternion implementation REFACTOR: Refactored quaternion implementation May 13, 2025
@codecov

codecov Bot commented May 13, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 94.43255% with 26 lines in your changes missing coverage. Please review.

Project coverage is 85.19%. Comparing base (e5917db) to head (79b4212).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6151      +/-   ##
==========================================
- Coverage   85.23%   85.19%   -0.04%     
==========================================
  Files         167      169       +2     
  Lines       63737    63991     +254     
==========================================
+ Hits        54325    54518     +193     
- Misses       9412     9473      +61     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Samuelopez-ansys

Copy link
Copy Markdown
Member

@Alberto-DM I like a lot this PR.

I have two requests, if I am not wrong these 2 new classes are not part of any section of the documentation, I think it would be great to add both. Maybe here:

https://aedt.docs.pyansys.com/version/stable/API/Primitives3D.html

or

https://aedt.docs.pyansys.com/version/stable/API/generic.html

And, I think it would be useful to add an example of in PyAEDT examples repository to show the power of this class (the quaternion one mainly), what do you think?

Maybe here: https://examples.aedt.docs.pyansys.com/version/dev/examples/aedt_general/modeler/index.html

@Alberto-DM

Copy link
Copy Markdown
Contributor Author

@Samuelopez-ansys, quaternions are powerful, but here are "just" used to convert between different coordinate systems. They are also used to translate a CS that is referring to another CS, which is referring to a third CS and so on.... It makes easy to get the equivalent rotation respect to Global.
All their use is behind the scene inside the CoordinateSystem classes. I never thought to use it directly.
If you think I can add a documentation page (if you tell me how to do it 😄 ).
Regarding the example I have to think what to show...

@Samuelopez-ansys

Copy link
Copy Markdown
Member

I think you could do something similar to this https://raw.githubusercontent.com/ansys/pyaedt/refs/heads/main/doc/source/API/generic.rst

But much simpleer, because just defining the class, it will automatically create the doc of all the methods of the class

Like here the farfield:

https://raw.githubusercontent.com/ansys/pyaedt/refs/heads/main/doc/source/API/visualization/advanced.rst

@lorenzovecchietti lorenzovecchietti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Everything looks correct to me. I've made just a couple of optional suggestions.

Achieving 100% coverage would be the icing on the cake ;)

Comment thread src/ansys/aedt/core/generic/quaternion.py
Comment thread src/ansys/aedt/core/generic/quaternion.py
Comment thread src/ansys/aedt/core/modeler/cad/modeler.py Outdated
@Alberto-DM

Copy link
Copy Markdown
Contributor Author

@lorenzovecchietti 94.43% coverage is the max I can do...

@Alberto-DM Alberto-DM enabled auto-merge (squash) May 15, 2025 12:36
@Alberto-DM Alberto-DM merged commit 2eeae49 into main May 15, 2025
34 checks passed
@Alberto-DM Alberto-DM deleted the Fix_coordinate_system_mode_change branch May 15, 2025 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants