JuliaAstro / SkyCoords.jl

Astronomical coordinate systems in Julia
https://juliaastro.github.io/SkyCoords.jl/stable
Other
26 stars 13 forks source link

Static arrays #10

Closed giordano closed 7 years ago

giordano commented 7 years ago

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 of Matrix33 × Matrix33. Instead multiplication of SMatrix{3,3} × SVector{3} should be more or less as fast as the multiplication Matrix33 × 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.

giordano commented 7 years ago

Tests don't pass on Julia 0.4. Time to drop support for it? See PR #11.

giordano commented 7 years ago

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%.

kbarbary commented 7 years ago

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.

giordano commented 7 years ago

It's nice to delete code, but this also adds a dependency.

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.

Any idea how stable StaticArrays is?

The syntax used here (SVector and SMatrix) has been stable for a while. The Julia Observer lists 52 packages directly requiring StaticArrays, and other 226 indirectly requiring it, so breaking changes will break a lot :-) You can also ask Jeff, StaticArrays are used in Celeste.

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.

No idea, but lately the trend in Julia base has been to remove packages from Base rather than importing new ones.

We may also need a minimum version on StaticArrays in REQUIRE.

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.

giordano commented 7 years ago

In case you missed, I updated the PR (in particular, the REQUIRE file) ;-)

With https://github.com/JuliaAstro/AstroLib.jl/pull/46 I'm using StaticArrays.jl also in AstroLib.jl. Thanks to this change I was able to reduce to zero memory allocations for many functions. However, in AstroLib.jl there are several different matrices layouts, making it impractical to define custom types for each of them, instead here in SkyCoords.jl we need only a 3 × 3 matrix and a 3-element vector.