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

upgrade mean of rotations to new LinearAlgebra syntax #90

Closed
wants to merge 4 commits into from
Closed

upgrade mean of rotations to new LinearAlgebra syntax #90

wants to merge 4 commits into from

Conversation

colinxs
Copy link

@colinxs colinxs commented Jan 29, 2019

mean was complaining about no eigfact function existing. This PR updates to current LinearAlgebra syntax which is eigvecs.

@codecov-io
Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #90 into master will decrease coverage by 15.04%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #90       +/-   ##
===========================================
- Coverage   86.99%   71.95%   -15.05%     
===========================================
  Files           9        8        -1     
  Lines         746      731       -15     
===========================================
- Hits          649      526      -123     
- Misses         97      205      +108
Impacted Files Coverage Δ
src/mean.jl 0% <0%> (ø) ⬆️
src/util.jl 40% <0%> (-50%) ⬇️
src/derivatives.jl 40.9% <0%> (-45.46%) ⬇️
src/quaternion_types.jl 52.99% <0%> (-42.74%) ⬇️
src/angleaxis_types.jl 71.95% <0%> (-20.74%) ⬇️
src/core_types.jl 74.19% <0%> (-12.55%) ⬇️
src/principal_value.jl 83.33% <0%> (-12.5%) ⬇️
src/euler_types.jl 83.52% <0%> (-0.97%) ⬇️
src/Rotations.jl

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 a5b1635...18a77bb. Read the comment docs.

Copy link
Contributor

@tkoolen tkoolen left a comment

Choose a reason for hiding this comment

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

Thanks! Would you mind adding a test? The reason this broke in the first place is that mean is not properly tested, so it would be great to prevent more breakage from happening in the future.

@@ -14,7 +14,7 @@ for axis in [:X, :Y, :Z]
@eval begin
struct $RotType{T} <: Rotation{3,T}
theta::T
$RotType{T}(theta) where {T} = new{T}(theta)
$RotType{T}(theta::Real) where {T} = new{T}(theta)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems unrelated and would break e.g. interop with SymPy.Sym. Please revert.

Copy link
Author

Choose a reason for hiding this comment

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

oops! haha I left that in by accident. There are some problems with mean() in its current form that I was trying to address. Namely that you cannot take the mean of two or three axis rotations. I'll file an issue for that!

Copy link
Author

Choose a reason for hiding this comment

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

and I'll definitely add a test. I'm a little slammed right now but in a day or two!

@andyferris
Copy link
Contributor

Is this one still relevant?

@hyrodium
Copy link
Collaborator

It seems adding tests is not finished, and the return value type of mean should be SMatrix.
#91 (comment)

@hyrodium
Copy link
Collaborator

hyrodium commented Nov 1, 2021

I'll close this PR. See #177, #178.

@hyrodium hyrodium closed this Nov 1, 2021
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.

5 participants