SunnySuite / Sunny.jl

Spin dynamics and generalization to SU(N) coherent states
Other
86 stars 19 forks source link

Hide plotting behind requires #6

Closed kbarros closed 2 years ago

kbarros commented 2 years ago

Here is a simple benchmark to time relative costs of "headless" vs "plotting" Sunny usage:

@time begin
using Sunny
using LinearAlgebra
lat_vecs = [1 1 0; 1 0 1; 0 1 1]' / 2
positions = [[1, 1, 1], [-1, -1, -1]] / 8
cryst = Crystal(lat_vecs, positions)
int = [exchange(1.0*I, Bond(1, 1, [1, 0, 0]))]
end

@time begin
using GLMakie # This line becomes required in the `requires` branch
plot_bonds(cryst, int, (2,2,2); markersize=200)
end

On main branch, the times are 13s and 42s. On requires branch, the times are 6.5s and 50s. So avoiding the using GLMakie shaves 50% off the time to do basic Sunny stuff. But then plotting becomes slower, and (if the user eventually wants to make a plot) there is a net cost increase of 56.5 - 55 = 1.5 seconds. I would personally consider this tradeoff worth it, but it should be discussed.

kbarros commented 2 years ago

Cole, incidentally, could you check that the plot the shows up is correct? It's a primitive fcc unit cell, but the dimensions look kind of strange to my eye.

ColeMiles commented 2 years ago

I can confirm similar timings on my machine (16s -> 8s, 54s -> 62s) -- this looks great! I think the hit to plotting time is worth the much faster time for everything else. Plus, it seems like the total time of loading a crystal + plotting it is roughly the same.

I'm wondering what the remaining big-hitters are for loading time. I guess CrystalInfoFramework or Spglib? We wouldn't want to hide either of these behind Requires though.

As for the plot -- it seems to be correct. I think there's just a lot of things going on which make it difficult to see -- the two-atom basis, the small number of unit cells, and I think it's just very difficult to get our brain to ignore the lines which are drawing the primitive unit cell.

Making positions = [[0, 0, 0]] and plotting 4x4x4 cells, I can just barely make out the FCC cube:

image

Even there the proportions look off, but if you rotate it around you can see it's a cube.

Anyway, seems good to merge to me.