search

JuliaMolSim / DFTK.jl

Density-functional toolkit
https://docs.dftk.org
MIT License
426 stars 89 forks source link

Refactor `plot_bandstructure` interface #503

Open mfherbst opened 3 years ago

mfherbst commented 3 years ago

I've become a little unhappy about the plot_bandstructure interface, especially the fact that the kline_density has so strange units. When Brillouin.jl is used this should be refactored.

thchr commented 2 years ago

When you have Brillouin.jl as a dependency, its plotting facilities might be relevant to this issue (if nothing else, maybe inspiration).

The design idea there was to keep the k-path as a structure and then use that to dispatch on to generate nice band structure plots. There's currently only one backend though (PlotlyJS.jl, accessed via Requires.jl).

mfherbst commented 2 years ago

Yes I have seen that, thanks. For sure the kpath and BZ plot might be very nice to expose. Perhaps also to integrate with you closer for the band structure plots.

I'm not sure if you are aware @thchr, but there currently is a bit of motion to build unified interfaces for materials modelling in Julia (e.g. see https://github.com/JuliaMolSim/AtomsBase.jl). This is all very much work in progress, but I think once that interface is a bit more settled it would be worth to revisit also ways to interface better between Brillouin and DFTK. I would definitely not mind to externalise some of our plotting ;).

thchr commented 2 years ago

Yeah, I had just briefly seen it the other day: cool effort, definitely!

One thing that came to mind when I was browsing the code of AtomsBase.jl was that it might be nice for various crystallography-related packages to settle on some standard "back-bone" for how to specify and transform direct and reciprocal lattices. The motivation would be to reduce the setting-related uncertainty that we also discussed in #496. The implementations in Bravais might be suitable in that regard (e.g., in place of Box).

mfherbst commented 2 years ago

The implementations in Bravais might be suitable in that regard (e.g., in place of Box).

Feel free to raise that aspect in AtomsBase.jl. Since AtomsBase is meant to be a bit more general (i.e. including molecular systems) I suppose it Bravais might not yet fit at the most general level, but I could imagine it to make sense to have a slightly more specialised implementation just for periodic problems ... and at that level Bravais could fit well, I agree.

mfherbst commented 2 years ago

See the code in https://github.com/thchr/Brillouin.jl/blob/master/src/requires/plotlyjs_dispersion.jl