Skip to content

Hide plotting behind requires #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

Merged
merged 2 commits into from
Nov 9, 2021
Merged

Hide plotting behind requires #6

merged 2 commits into from
Nov 9, 2021

Conversation

kbarros
Copy link
Member

@kbarros kbarros commented Nov 7, 2021

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
Copy link
Member Author

kbarros commented Nov 9, 2021

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
Copy link
Contributor

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.

@ColeMiles ColeMiles merged commit 9f7415a into main Nov 9, 2021
@ColeMiles ColeMiles deleted the requires branch November 9, 2021 04:54
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.

2 participants