Brillouin zone plotter

[zccaayo]

Script to plot the high symmetry points on the Brillouin zone.

[utf]

Thanks for this, implementation looks nice and clean!

Main comment is can you make this pep8 compatible (i.e. 4 spaces for an indent, line length not longer than 80 characters).

You could do this automatically by running the file through a python formatter. I.e. https://pypi.org/project/black/

Literally just

pip install black
black -l 80 brillplot.py

It will make the formatting a little strange but tbh I am considering running black on the whole of sumo at some point.

[ajjackson]

Would be nice if this wasn't 100% Vasp-oriented. All that is needed is a Pymatgen bandstructure object, and we can get those for Questaal too... (and Castep is in another branch but band structures already work)

I can make the adaptations if you like.

[utf]

I agree with Adam, the script should ideally be code agnostic!

[ajjackson]

It shouldn't be too tough to add style support or a window pop-up. Do we have an argument to plot to screen on any of the other sumo programs?

[ajjackson]

Otherwise this is a good start, well done!

[ajjackson]

I can't contribute changes in this PR, so they have to be done on your side. In future, make changes on a separate branch (not master) of your fork, and when you make the PR check the tickbox to "allow edits from maintainers".

It should still be possible to enable that from your side, but because you are working on master it is likely the git history is going to get pretty messy in the near future (i.e. if we add anything to master before this get merged). I'd be happy to accept this PR without the style features and then add them myself in a separate PR.

If you look at "details" next to the failed Codacy check, we see that it is complaining about an unused "import argparse". I don't really understand how the code can work if that is unused. How does

	def _get_parser():
	    parser.add_argument(

know what parser is? I think we're missing a

parser = argparse.ArgumentParser(description=..., ...)```
Have a look at bandplot and make sure that's all working ok.

[zccaayo]

Sorry, I’m still getting used to the etiquette of GitHub. I think I have added you and Alex to the repository. Given that my changes don’t really affect the other codes in the package I didn’t think it was a massive issue to work on master. Should I start another branch or should I keep going with adding the changes in master?

[ajjackson]

No worries, it does take a little practice!

If you change branch we'd have to reopen the PR. The issue with master is that your fork master will get a different history from the main master, at which point your updates by pulling from upstream will start to fail. Best bet is to stick to your master for now, and when this PR is done you can use git reset to get your local master lined up with the main repo again.

[ajjackson]

The plotter isn't actually working for me? savefig fails because plt is None; BSPlotter.plot_brillouin() doesn't actually return anything? https://pymatgen.org/_modules/pymatgen/electronic_structure/plotter.html#BSPlotter.plot_brillouin

It might make more sense to replace BSPlotter.plot_brillouin() with our own version that directly calls plot_brillouin_zone() or plot_brillouin_zone_from_kpath(); these functions would also allow us to pass in an Axis object for better control over the plotting arrangement. https://pymatgen.org/_modules/pymatgen/electronic_structure/plotter.html#plot_brillouin_zone_from_kpath.

By the way, you have some tab characters in the file. Please set up your editor to use spaces instead of tabs when editing Python; I've converted the ones I've found.

[zccaayo]

So basically, it throws up a gui when you call BSPlotter(bs).plot_brillouin() but doesn't return a matplotlib object which can be saved?

[ajjackson]

Ah right, so that's why it's useless with the Agg backend. I suggest we a) make it work by directly drawing to an Axis using Agg as described above and b) open an Issue to discuss making pop-up plots work. There are some fundamental Matplotlib challenges to deal with there and we should make the approach consistent across Sumo.

[zccaayo]

import os
import sys
import glob
import logging
import pymatgen

from pymatgen.io.vasp.outputs import BSVasprun
from pymatgen.electronic_structure.bandstructure import \
    get_reconstructed_band_structure
from pymatgen.electronic_structure.plotter import BSPlotter as plotter
from pymatgen.electronic_structure.plotter import plot_brillouin_zone_from_kpath as kpath

import matplotlib as mpl
mpl.use('TkAgg')
folders = glob.glob('split-*')
folders = sorted(folders) if folders else ['.']

filenames = []
for fol in folders:
        vr_file = os.path.join(fol, 'vasprun.xml')
        vr_file_gz = os.path.join(fol, 'vasprun.xml.gz')
        if os.path.exists(vr_file):
            filenames.append(vr_file)
        elif os.path.exists(vr_file_gz):
            filenames.append(vr_file_gz)
        else:
            logging.error('ERROR: No vasprun.xml found in {}!'.format(fol))
            sys.exit()

bandstructures = []
for vr_file in filenames:
        vr = BSVasprun(vr_file)
        bs = vr.get_band_structure(line_mode=True)
        bandstructures.append(bs)
bs = get_reconstructed_band_structure(bandstructures)

labels = {}
for k in bs.kpoints:
    if k.label:
        labels[k.label] = k.frac_coords
    lines = []
for b in bs.branches:
    lines.append([bs.kpoints[b['start_index']].frac_coords,
        bs.kpoints[b['end_index']].frac_coords])

plt = pymatgen.electronic_structure.plotter.plot_brillouin_zone(bs.lattice_rec, lines=lines, labels=labels)
plt.savefig("brill.pdf")

This is a very rough code which actually does print out a .pdf

[ajjackson]

Ok, I've made that sort-of-work in the Brillplot program. The next step would be to get styling to work, and as you can see we already have a merge conflict. We also don't have any of the styling infrastructure on this branch, presumably because your master was rather stale when beginning work on this.

Ordinarily this would be solved by merging from master, but that's going to be rather messy in this case because we are on a branch that is also called master. The best case is to rebase this branch on top of origin/master, which essentially "replays" the changes on top of a more up-to-date codebase. Then (after fixing conflicts), we would not normally be allowed to push them to this branch, because we've changed history and it could cause other people to lose work when they pull. We can "force-push" to get around this restriction, but it should be done with care.

I'll do just that in a moment. do not pull if you have work that can't be recovered when the history of master is rewritten. (If you are in that position, get that work onto another named branch so you can still find it.)

[ajjackson]

Done! It's a bit dirty rebasing a "master" like this (and breaks the history with @utf 's useful review comments), but rebasing/force-pushing like this can be seen as helpful for "feature branches" (preferably before review) as it keeps the git history fairly clean (with fewer merges) in the long-term, and makes it easier to see what each PR is adding.

[zccaayo]

I like what you've done with the pretty plot 3D plotter, Adam. It works a treat for me!

[ajjackson]

Good good! Some remaining things to consider:

• Docs: as a new command-line tool this will need its own section in the sphinx docs. Do you know how to edit/build those? Basically with the sphinx dependencies installed (there's a magic command to do that with Pip mentioned in the Sumo installation instructions) go into the docs folder and make html to get a web page with the current version of the docs. To edit the docs, work on the .rst files and commit to Git just like any other source code file. We may want to get it looking a bit prettier before adding to the examples gallery though...

• Styling: have a look at how the other bits of Sumo do it. Basically we made a "decorator" @styled_plot(sumo_base_style, other_styles, ...) which goes above the definition of a plotting function. As we are directly calling the Pymatgen plotter here, we should wrap that part in a plotting function (with fonts=, style= and no_base_style= options) that this can decorate. This decorator automatically reads those arguments and deals with them. I can do this if you're not comfortable with it.

• Support for other codes: I'm happy to have that be a separate PR after this one is merged

• Tests: we could have a test which checks out the returned plt object and makes sure it has lines on it etc. At some point we should do something more rigorous but that's a sumo-wide issue; comparing image files is very fragile against changes in the machine used and the Matplotlib version. I'd tolerate merging this PR without any new tests as it really is just a Pymatgen wrapper that doesn't add much processing.

[zccaayo]

Thanks for this! I need a little bit to fully digest those suggestions. I will start with item one and see how far I get.