Skip to content

[math] implement genvector custom axis rotation #18462

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 10 commits into from
May 6, 2025

Conversation

ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Apr 22, 2025

This Pull request:

Changes or fixes:

Fixes https://root-forum.cern.ch/t/tvector3-rotate-to-arbitrary-rotation-using-xyzvector/63244

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@ferdymercury ferdymercury requested a review from mdessole April 22, 2025 13:24
Copy link

github-actions bot commented Apr 22, 2025

Test Results

    19 files      19 suites   4d 10h 23m 26s ⏱️
 2 731 tests  2 731 ✅ 0 💤 0 ❌
50 450 runs  50 450 ✅ 0 💤 0 ❌

Results for commit 96fb2e8.

♻️ This comment has been updated with latest results.

@ferdymercury ferdymercury marked this pull request as ready for review April 22, 2025 16:23
@ferdymercury ferdymercury requested a review from lmoneta as a code owner April 22, 2025 16:23
@ferdymercury ferdymercury added this to the 6.38.00 milestone Apr 22, 2025
@chrishanw
Copy link

chrishanw commented Apr 30, 2025

Thanks for this PR. As I wrote in the forum post, the new Rotate function solves the issue for rotating a XYZVector around an arbitrary axis.

Edit: Of course the other solution implemented in #18480 will also solve the issue by enabling the usage of Rotation3D, Thanks!

Copy link
Member

@hageboeck hageboeck left a comment

Choose a reason for hiding this comment

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

Hello @ferdymercury,

thanks for taking this up! Also here, having a small test would make it easier to judge what's going on.

One more function in https://github.com/root-project/root/blob/master/math/genvector/test/stress3D.cxx maybe.
Better here: https://github.com/root-project/root/blob/master/math/genvector/test/testGenVector.cxx

@ferdymercury ferdymercury dismissed hageboeck’s stale review May 5, 2025 07:07

Added, thanks!

@ferdymercury ferdymercury requested a review from hageboeck May 5, 2025 07:07
Copy link
Member

@hageboeck hageboeck left a comment

Choose a reason for hiding this comment

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

Thanks @ferdymercury! LGTM!
Let's wait for the builds to complete.

@ferdymercury ferdymercury requested a review from bellenot May 5, 2025 13:39
@mdessole mdessole merged commit 703a315 into root-project:master May 6, 2025
22 of 23 checks passed
@ferdymercury ferdymercury deleted the genvecrotate branch May 6, 2025 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants