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.

Sign up for GitHub

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

Jump to bottom

Update fields of UnitQuaternion #191

Merged
merged 10 commits into from
Nov 16, 2021
Merged

Update fields of UnitQuaternion #191

merged 10 commits into from
Nov 16, 2021

Conversation

hyrodium
Copy link
Collaborator

@hyrodium hyrodium commented Nov 8, 2021

This PR branch is based on #175, so please merge the PR first.

In the previous PR, I have written the following.

  • UnitQuaternoin{T} sitll have fields w::T, x::T, y::T and z::T, not q::Quaternion{T}.
    • I'd like to keep this PR small, so this field will be changed in another PR.

This PR fixes the problem.

WIP, not in this PR

  • The name of unit quaternion type is still UnitQuaternion.
    • This will be changed to QuatRotation in the future.
  • The return value type of _pure_quaternion is Quaternion.
    • This should be InfinitesimalQuatRotation <: InfinitesimalRotation in the future.

@hyrodium hyrodium requested a review from andyferris November 8, 2021 14:55
@codecov
Copy link

codecov bot commented Nov 8, 2021 •
edited
Loading

Codecov Report

Merging #191 (19c67e2) into master (15d1c32) will increase coverage by 0.80%.
The diff coverage is 97.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   86.12%   86.93%   +0.80%     
==========================================
  Files          14       14              
  Lines        1341     1401      +60     
==========================================
+ Hits         1155     1218      +63     
+ Misses        186      183       -3
Impacted Files Coverage Δ
src/util.jl 80.00% <ø> (ø)
src/error_maps.jl 93.19% <86.66%> (+0.55%) ⬆️
src/angleaxis_types.jl 94.05% <100.00%> (+0.24%) ⬆️
src/derivatives.jl 100.00% <100.00%> (ø)
src/mrps.jl 35.37% <100.00%> (+1.80%) ⬆️
src/principal_value.jl 100.00% <100.00%> (ø)
src/rodrigues_params.jl 98.66% <100.00%> (+0.05%) ⬆️
src/unitquaternion.jl 96.20% <100.00%> (+2.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15d1c32...19c67e2. Read the comment docs.

andyferris
andyferris approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@andyferris andyferris left a comment

Choose a reason for hiding this comment

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

Cool :)

Would it be worth renaming all the ws to ss to make the code a bit clearer?

Project.toml Outdated
@@ -18,7 +19,8 @@ ForwardDiff = "f6369f11-7733-5829-9624-2563aa707210"
InteractiveUtils = "b77e0a4c-d291-57a0-90e8-8db25a27a240"
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
Quaternions = "94ee1d12-ae83-5a48-8b1c-48b8ff168ae0"
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need it here as well as above (same for Random)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I also removed Quaternions and Random from targets/test.

@hyrodium hyrodium force-pushed the feature/correct_quaternion2 branch from b687eb2 to 21d588b Compare November 10, 2021 13:35
hyrodium
hyrodium commented Nov 10, 2021
View reviewed changes
src/derivatives.jl
@@ -134,16 +138,21 @@ end
# Jacobian converting from a Quaternion to an MRP
#
function jacobian(::Type{MRP}, q::UnitQuaternion{T}) where T
den = 1 + q.w
w = q.q.s
Copy link
Collaborator Author

@hyrodium hyrodium Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Would it be worth renaming all the ws to ss to make the code a bit clearer?

@andyferris Are you suggesting changes like this?

Suggested change
w = q.q.s
s = q.q.s

We are using (w,x,y,z) for symbols for coefficients of a quaternion of UnitQuaternion, and (s,v1,v2,v3) for Quaternion. So, I would like to keep ws here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I'll merge this PR.
If there is some problem with this, please open an issue about this.

@hyrodium hyrodium mentioned this pull request Nov 14, 2021
Closed
@hyrodium hyrodium merged commit 2e02102 into JuliaGeometry:master Nov 16, 2021
@hyrodium hyrodium mentioned this pull request Nov 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyferris andyferris andyferris approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hyrodium @andyferris