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

Use Rotations.jl for rotations #415

Merged
merged 37 commits into from
May 26, 2023

Conversation

hyrodium
Copy link
Contributor

@hyrodium hyrodium commented May 5, 2023

This PR fixes #344.
I'm keeping this PR as a draft because removing ReferenceFrameRotations.DCM is not finished yet.
I just would like to share the progress and get some feedback on the changes on this PR.

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #415 (4caa453) into master (1ce5888) will increase coverage by 0.04%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
+ Coverage   93.81%   93.85%   +0.04%     
==========================================
  Files         121      120       -1     
  Lines        3522     3501      -21     
==========================================
- Hits         3304     3286      -18     
+ Misses        218      215       -3     
Impacted Files Coverage Δ
src/Meshes.jl 100.00% <ø> (ø)
src/utils.jl 100.00% <ø> (ø)
src/neighborhoods/metricball.jl 63.63% <80.00%> (-5.33%) ⬇️
src/primitives/torus.jl 84.00% <100.00%> (ø)
src/sampling/regular.jl 100.00% <100.00%> (ø)
src/transforms/rotate.jl 69.23% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@juliohm
Copy link
Member

juliohm commented May 5, 2023

Looking forward to these improvements @hyrodium ❤️

It would be nice if you could summarize the new types from Rotations.jl that replace the old types from ReferenceFrameRotations.jl in a comment here when you are done 👌🏽

@hyrodium hyrodium marked this pull request as ready for review May 6, 2023 14:13
Copy link
Contributor Author

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

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

This PR is ready to review! I added some comments for the changes.

src/transforms/rotate.jl Show resolved Hide resolved
src/neighborhoods/metricball.jl Outdated Show resolved Hide resolved
test/rotations.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/transforms/rotate.jl Outdated Show resolved Hide resolved
src/neighborhoods/metricball.jl Outdated Show resolved Hide resolved
src/rotations.jl Show resolved Hide resolved
src/rotations.jl Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

Lovely PR @hyrodium , it is almost ready.

Project.toml Show resolved Hide resolved
src/Meshes.jl Outdated Show resolved Hide resolved
src/neighborhoods/metricball.jl Outdated Show resolved Hide resolved
src/neighborhoods/metricball.jl Outdated Show resolved Hide resolved
src/neighborhoods/metricball.jl Outdated Show resolved Hide resolved
src/transforms/rotate.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
test/rotations.jl Outdated Show resolved Hide resolved
src/Meshes.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
@juliohm
Copy link
Member

juliohm commented May 7, 2023

@hyrodium I am available today in case you want to finish this PR for a final review. Thanks again for the great improvement.

@juliohm
Copy link
Member

juliohm commented May 16, 2023

@hyrodium can you please rebase this PR on top of the latest master branch?

@juliohm
Copy link
Member

juliohm commented May 22, 2023

@hyrodium any chance we could get this merged this week? We are planning a new breaking release and it would be nice to incorporate the changes here.

@hyrodium
Copy link
Contributor Author

Sorry for being late. I have merged the current master branch to resolve the conflicts.

@hyrodium
Copy link
Contributor Author

I will update this PR in a few days to reflect the review comments.

src/transforms/rotate.jl Outdated Show resolved Hide resolved
src/transforms/rotate.jl Outdated Show resolved Hide resolved
src/transforms/rotate.jl Outdated Show resolved Hide resolved
test/utils.jl Outdated Show resolved Hide resolved
@juliohm juliohm merged commit 25e7061 into JuliaGeometry:master May 26, 2023
@juliohm
Copy link
Member

juliohm commented May 26, 2023

Thank you @hyrodium ! ❤️

@hyrodium hyrodium deleted the feature/Rotations.jl branch May 26, 2023 01:11
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.

Improve rotation standards
2 participants