Skip to content

REFACTOR: Refactored quaternion implementation #6151

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

Merged
merged 31 commits into from
May 15, 2025

Conversation

Alberto-DM
Copy link
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
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
Copy link

codecov bot commented May 13, 2025

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
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
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
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

Copy link
Contributor

@lorenzovecchietti lorenzovecchietti left a comment

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 ;)

@Alberto-DM
Copy link
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