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

Juliav0.7 fixes #61

Merged
merged 7 commits into from
Jul 3, 2018
Merged

Juliav0.7 fixes #61

merged 7 commits into from
Jul 3, 2018

Conversation

slmcbane
Copy link
Contributor

@slmcbane slmcbane commented Jul 3, 2018

Very small PR for v0.7 update; the only code change is substituting norm for vecnorm (deprecated). I'm not sure if my changes for CI and REQUIRE are correct as I don't have experience with package development myself.

@tkoolen
Copy link
Contributor

tkoolen commented Jul 3, 2018

Ah thanks, I missed that one before tagging the new version.

Maybe it would be better to use Compat.norm from Compat.jl (already a dependency of Rotations), so that we can keep 0.6 compatibility for a bit longer. I think that as long as it's easy to keep compatibility, we should hold off on dropping 0.6.

@slmcbane
Copy link
Contributor Author

slmcbane commented Jul 3, 2018

You're right, done. I also edited so 0.6, 0.7, and nightly are all tested and only nightly allows failures.

@slmcbane
Copy link
Contributor Author

slmcbane commented Jul 3, 2018

Realized I goofed when I changed to Compat... my eyes are glazing over. Should be fixed shortly.

Compat.norm still throws depwarns in v0.7, I wrapped the using in a version check.

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, added some comments.

REQUIRE Outdated
@@ -1,3 +1,3 @@
julia 0.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo?

@@ -137,12 +137,12 @@ function isrotation(r::AbstractMatrix{T}, tol::Real = 1000 * eps(eltype(T))) whe
# Transpose is overloaded for many of our types, so we do it explicitly:
r_trans = @SMatrix [conj(r[1,1]) conj(r[2,1]);
conj(r[1,2]) conj(r[2,2])]
d = vecnorm((r * r_trans) - eye(SMatrix{2,2}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just explicitly call Compat.norm here instead of the conditional import in src/Rotations.jl.

src/Rotations.jl Outdated
@@ -8,14 +8,18 @@ using Compat.LinearAlgebra
using StaticArrays

import Base: convert, eltype, size, length, getindex, *, Tuple
import Compat.LinearAlgebra: inv, eye
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo?

@slmcbane
Copy link
Contributor Author

slmcbane commented Jul 3, 2018

I think I fixed all of those now, not sure what I was thinking when I removed the Compat import.

elseif size(r) == (3,3)
r_trans = @SMatrix [conj(r[1,1]) conj(r[2,1]) conj(r[3,1]);
conj(r[1,2]) conj(r[2,2]) conj(r[2,3]);
conj(r[1,3]) conj(r[2,3]) conj(r[3,3])]
d = vecnorm((r * r_trans) - eye(SMatrix{3,3}))
d = norm((r * r_trans) - eye(SMatrix{3,3}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Compat.norm here as well.

@slmcbane
Copy link
Contributor Author

slmcbane commented Jul 3, 2018

Done

@tkoolen tkoolen merged commit b7d5a12 into JuliaGeometry:master Jul 3, 2018
@tkoolen
Copy link
Contributor

tkoolen commented Jul 3, 2018

Thank you!

@slmcbane slmcbane deleted the juliav0.7-fixes branch July 4, 2018 12:24
@tkoolen
Copy link
Contributor

tkoolen commented Jul 4, 2018

Ah oops, needs a new Compat version tag that includes JuliaLang/Compat.jl#577, my fault.

@slmcbane slmcbane restored the juliav0.7-fixes branch July 4, 2018 15:30
@slmcbane
Copy link
Contributor Author

slmcbane commented Jul 4, 2018

It looks like 0.69 is the latest Compat version?

@tkoolen
Copy link
Contributor

tkoolen commented Jul 5, 2018

Yeah, we'll have to wait for a new version to come out. No worries, Compat versions get tagged pretty frequently, we can wait for that before a new Rotations release.

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.

2 participants