-
Notifications
You must be signed in to change notification settings - Fork 13
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
Static arrays #10
Static arrays #10
Conversation
Tests don't pass on Julia 0.4. Time to drop support for it? See PR #11. |
Running the benchmark script shown at https://github.com/kbarbary/SkyCoords.jl/pull/9#issuecomment-336395556 I get the following result on this branch with Julia master: n,system,time
1,galactic,7.944524793388431e-8
1,fk5j2000,1.56615e-7
1,fk5j1975,1.668919631093544e-7
10,galactic,8.326533333333333e-7
10,fk5j2000,9.68576923076923e-7
10,fk5j1975,9.509130434782609e-7
100,galactic,8.332333333333333e-6
100,fk5j2000,8.499666666666666e-6
100,fk5j1975,8.549e-6
1000,galactic,0.000103926
1000,fk5j2000,0.000105402
1000,fk5j1975,0.000105456
10000,galactic,0.001076566
10000,fk5j2000,0.001083351
10000,fk5j1975,0.001081381
100000,galactic,0.010805647
100000,fk5j2000,0.010876967
100000,fk5j1975,0.01086324
1000000,galactic,0.108722475
1000000,fk5j2000,0.109348184
1000000,fk5j1975,0.10926184 This is always an improvement compared to master (1-22%), instead compared to PR #9 there is sometimes an improvement and sometimes a regression. For n >= 100 the regression is never more than ~1%. |
c5f5af5
to
ff14971
Compare
It's nice to delete code, but this also adds a dependency. Any idea how stable StaticArrays is? And do you know if there is a plan to put static arrays in Base at some point? I haven't been following the work there. We may also need a minimum version on StaticArrays in REQUIRE. |
Which depends only on Compat, so the dependency is almost as light as it can be. Besides deleting code, the other advantage is to get even more optimized operations, like matrix multiplications, even though this isn't very relevant at run-time. If matrix × vector multiplication will be made faster (if possible at all) the package will readily take advantage of the improvement.
The syntax used here (
No idea, but lately the trend in Julia base has been to remove packages from Base rather than importing new ones.
Yes, didn't add before also because StaticArrays was incompatible with Julia 0.4. I'll probably pick v0.3.1, the latest supporting Julia 0.5. |
0835d77
to
5600e37
Compare
In case you missed, I updated the PR (in particular, the REQUIRE file) ;-) With JuliaAstro/AstroLib.jl#46 I'm using |
5600e37
to
8dbd79c
Compare
This PR is based on top of #9, so should wait that one before merging this. I kept them separated because are logically independent.
Multiplication of
SMatrix{3,3} × SMatrix{3,3}
should be faster than the multiplication ofMatrix33 × Matrix33
. Instead multiplication ofSMatrix{3,3} × SVector{3}
should be more or less as fast as the multiplicationMatrix33 × Vector3
, and this operation is performed at runtime.According to my tests, this PR should leave overall performance very close to #9, the main advantage is to drop the custom types and reduce the code to maintain. Performance should improve compared to current master. You may want to carry out some benchmarks before applying this.