-
Notifications
You must be signed in to change notification settings - Fork 6
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
Looks cool, but way slower #23
Comments
Thank you for looking into this. I've been very ambivalent about this package for quite some time. On the one hand it's nifty but almost always I end up using radians everywhere and that's that. The only time I really found a real need for this is when humans IO was involved, in the form of GUI or plots. |
Just for bookkeeping's sake, we need to:
|
I think the reason the import statement used in the docs doesn't work is because neither the degree symbol nor rad are defined in this code. They seem to both be defined in unitful itself, so the following works for me: import Unitful: °, rad |
#32 fixes the issues with the import statements. |
I found the culprit of the slowdown! It is the use of rational numbers (with defining: @unit octant "octant" Octant turn//8 false
@unit octant_f "octant_f" Octant_f turn/8 false Then: julia> using BenchmarkTools
julia> using UnitfulAngles: °, rad, octant, octant_f
@btime sin(1)
0.001 ns (0 allocations: 0 bytes)
0.8414709848078965
julia> @btime sin(1rad)
0.001 ns (0 allocations: 0 bytes)
0.8414709848078965
julia> @btime sin(1°)
0.001 ns (0 allocations: 0 bytes)
0.01745240643728351
julia> @btime sin(1octant)
90.691 ns (0 allocations: 0 bytes)
0.7071067811865476
julia> @btime sin(1octant_f)
0.001 ns (0 allocations: 0 bytes)
0.7071067811865475 |
Understood. Sorry for the influx of comments and PRs... 😄 I'm learning Julia and have been using the different packages in the Unitful ecosystem to learn how to write good Julia, write packages, etc. For what it's worth, I think this is a neat little package worth maintaining. Thanks! |
Great detective work @cmichelenstrofer ! |
If you use a float rather than an integer then there is no performance issues: @btime sin(1.0octant)
0.001 ns (0 allocations: 0 bytes)
0.7071067811865476 Since there is already a way to get fast performance I would say there is no need to duplicate the units with |
This seems to be more an issue with Unitful than with Julia: julia> @btime(0.125)
0.001 ns (0 allocations: 0 bytes)
0.125
julia> @btime(1//8)
0.001 ns (0 allocations: 0 bytes)
1//8 See this comment in PainterQubits/Unitful.jl#281 which points to the offending line in the Unitful code. However this is considered a feature:
In that same comment he recommends simply passing in floats for fast code:
Given that this is expected behavior from Unitful, and there is a workaround (using floats), I would say that no change to UnitfulAngles is needed. This really should be documented in Unitful though. |
You'll need to be careful with timings well below 1ns. julia> @btime sin(1rad)
0.001 ns (0 allocations: 0 bytes)
0.8414709848078965 is completely calculated at the compiler. julia> @btime $(Ref(1))[] // $(Ref(8))[] * $(Ref(2.1))[]
13.367 ns (0 allocations: 0 bytes)
0.2625
julia> @btime $(Ref(0.125))[] * $(Ref(2.1))[]
0.977 ns (0 allocations: 0 bytes)
0.2625 |
I agree. Since this issue originates elsewhere and shouldn't be resolved out of scope here, I'll close this for now. Feel free to reopen if you disagree. |
@zsoerenm that is my mistake, I copied the wrong lines from the REPL! I meant to show this: julia> @btime cos(0.125)
0.875 ns (0 allocations: 0 bytes)
0.992197667229329
julia> @btime cos(1//8)
0.875 ns (0 allocations: 0 bytes)
0.992197667229329 Here are some new benchmarks using the method you shared (hopefully I'm using it right). The conclusions above are unchanged. julia> @btime sin($(Ref(1))[])
10.917 ns (0 allocations: 0 bytes)
0.8414709848078965
julia> @btime sin($(Ref(1rad))[])
10.917 ns (0 allocations: 0 bytes)
0.8414709848078965
julia> @btime sin($(Ref(1octant))[])
95.588 ns (0 allocations: 0 bytes)
0.7071067811865476
julia> @btime sin($(Ref(1.0octant))[])
6.041 ns (0 allocations: 0 bytes)
0.7071067811865476
julia> @btime sin($(Ref(1//8))[])
3.041 ns (0 allocations: 0 bytes)
0.12467473338522769
julia> @btime sin($(Ref(0.125))[])
3.041 ns (0 allocations: 0 bytes)
0.12467473338522769 Also, thank you for pointing out the right way to use |
@yakir12 based on the comment
in the trig functions, before calling current behavior: julia> @btime sin($(Ref(1octant))[])
97.251 ns (0 allocations: 0 bytes)
0.7071067811865476 converting to float before julia> @btime sin($(Ref(1octant))[])
5.833 ns (0 allocations: 0 bytes)
0.7071067811865476 I'll submit a PR with this proposed solution. |
I also wanted to let you know the import from the docs didn't work. I may look into both later, just wanted to file so I don't forget.
The text was updated successfully, but these errors were encountered: