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

Fix #15, error with latest StaticArrays due to dot change. #16

Merged
merged 3 commits into from
Jun 20, 2019

Conversation

tkoolen
Copy link
Collaborator

@tkoolen tkoolen commented Jun 17, 2019

CC: @goretkin.

Microbenchmark results: on Julia 1.1.1:

julia> @btime transpose(a) * b setup = begin
           a = rand(SVector{2})
           b = SVector(rand(SVector{2}), rand(SVector{2}))
           end
  10.521 ns (0 allocations: 0 bytes)

which at least doesn't allocate, but on latest master:

julia> @btime transpose(a) * b setup = begin
           a = rand(SVector{2})
           b = SVector(rand(SVector{2}), rand(SVector{2}))
           end
  1.762 ns (0 allocations: 0 bytes)

I think the difference is that an assertion error is being 'outlined' automatically in newer versions. I think this is the cleanest solution going forward, but if you consider this to be too much of a performance bug on 1.1, I can change it.

@goretkin
Copy link
Contributor

Thanks! Did you bump the StaticArrays version because this change isn't compatible with older versions?

I was going to suggest first restricting StaticArrays at v0.5.1-v0.10, and then applying a change that makes it compatible with v0.11, but it seems that Pkg does not support version ranges yet: JuliaLang/Pkg.jl#843

@tkoolen
Copy link
Collaborator Author

tkoolen commented Jun 18, 2019

Good point, I guess bumping the StaticArrays lower bound isn't necessary. Pkg does support version ranges, just not spelled that way. The 'correct' thing to do is to retroactively upper bound the StaticArrays version on previous releases, but if we just release a new version with the fix then just doing a pkg> up will be fine.

I'm going to replace this implementation with a generated function to get the same performance on 1.1 later today. I had trouble getting down to the performance on master without using a generated function.

Also, not sure what's going on on Julia 1.0 Travis builds.

@goretkin
Copy link
Contributor

Pkg does support version ranges, just not spelled that way

Do you mean like: JuliaLang/Pkg.jl#1232 (comment) , or something else?

@codecov-io
Copy link

codecov-io commented Jun 18, 2019

Codecov Report

Merging #16 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   93.26%   93.42%   +0.15%     
==========================================
  Files           8        8              
  Lines         208      213       +5     
==========================================
+ Hits          194      199       +5     
  Misses         14       14
Impacted Files Coverage Δ
src/gjk.jl 95.45% <100%> (+0.37%) ⬆️
src/reference_distance.jl 95% <100%> (ø) ⬆️

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 f6a6553...19489ae. Read the comment docs.

@tkoolen
Copy link
Collaborator Author

tkoolen commented Jun 18, 2019

OK, switched to the generated function approach, which is fast for both current and future versions of Julia.

As for the Travis failures on Julia 1, I can reproduce them locally with Julia 1.0.4. only with code coverage turned on! So I think this is another instance of JuliaLang/julia#30872.

@tkoolen
Copy link
Collaborator Author

tkoolen commented Jun 18, 2019

@rdeits, would you be OK with allowing failures on 1.0?

@tkoolen
Copy link
Collaborator Author

tkoolen commented Jun 20, 2019

Went ahead with allowing failures on 1.0.

@tkoolen tkoolen merged commit 526f46a into master Jun 20, 2019
@tkoolen tkoolen deleted the tk/staticarrays-compat branch June 20, 2019 16:52
@tkoolen
Copy link
Collaborator Author

tkoolen commented Jun 20, 2019

@goretkin, I started the process for tagging a new version: JuliaRegistries/General#1483.

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.

3 participants