Skip to content
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

[Feature Discussion] Wannier90 support #218

Open
oashour opened this issue Jul 23, 2023 · 3 comments
Open

[Feature Discussion] Wannier90 support #218

oashour opened this issue Jul 23, 2023 · 3 comments

Comments

@oashour
Copy link
Contributor

oashour commented Jul 23, 2023

One more issue/PR after this one!

(TL;DR at the bottom)

I've been doing some work with Wannier90 lately and didn't want to plot without my favorite package, so I have been working on implementing Wannier90 support. I think it makes the most sense to make that a pymatgen namespace package that provides all the data types Sumo works with (structures, lattices, band structures, etc.), just like pymatgen.io.espresso. This gives the package room to grow and decouples the development from sumo (I currently only plan to implement the stuff sumo needs, other features will come later.)

I have more or less already written the core of the package, and it works perfectly with SBSPlotter. I just need to implement the CLI, it will involve minimal changes like the proposed Quantum ESPRESSO support. The only new thing I want to implement is an option to automatically add the Wannierization windows (outer window and inner/frozen windows) to the plot. It's a lot easier to tweak the windows in the calculation and analyze the match/mismatch of bands when you can actually see them with your bands.

I'm thinking of a --show-windows flag to sumo-bandplot (or something along those lines), and using the new **kwargs implemented in draw_themed_line.

Here's an example using the comparison feature I've been working on (the top dashed line is the max energy of the frozen window).
image

TL;DR I'm opening this issue to solicit feedback. This feature will only require minimal changes to sumo, all the parsing will be done by a separate package.

@ajjackson
Copy link
Member

This seems reasonable. Will it be easy to test?

@oashour
Copy link
Contributor Author

oashour commented Jul 27, 2023

I guess most of the unit tests would sit in whatever package I end up writing to handle the actual parsing of the Wannier90 calculations, it would be an identical situation to the proposed Quantum ESPRESSO support. I guess it would be tested the same way sumo would test plotting from any other code, but I don't think there are any plotting-specific unit tests in sumo right now.

(I actually have no idea how to write plotting-specific unit tests, Julia's Plots.jl has this idea of plotting recipes that I used when unit testing Julia plots, but I don't know if matplotlib has anything similar).

@ajjackson
Copy link
Member

Plot testing is really hard! There is at least one package out there for matplotlib that tries to normalise the less predictable parameters (screen resolution etc), save images and then compare them.

For Sumo tests we don't render the plot but have a poke around the ax.lines etc. to see if the data looks correct.

As more code interfaces are added it doesn't make sense to test the actual plots; we should just check the data that makes it to a plotter.

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

No branches or pull requests

2 participants