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] Comparing band structures on the same plot #219

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

[Feature Discussion] Comparing band structures on the same plot #219

oashour opened this issue Jul 23, 2023 · 7 comments

Comments

@oashour
Copy link
Contributor

oashour commented Jul 23, 2023

Last issue/PR of the night, I promise! :)

I've recently needed to compare electronic band structures computed from the same structures/materials but with slightly different methods (e.g., VASP versus different pseudo potentials in Quantum ESPRESSO vs Wannier90). This work is available on my compare branch.

Currently Implemented:

  • Ability to plot multiple band structures on the same plot by passing SBSPlotter an array of BandStructureSymmLine objects, like the base BSPlotter from pymatgen.
  • Ability to add a legend for these cases via the bs_legend argument to SBSPlotter.get_plot. I don't think this comparison feature makes sense for projected band structures and don't plan to implement it.
  • Sort of unrelated but it was really easy to add while working on this feature: a legend for spin-polarized band structures to help differentiate spin up and down bands. Accessible via the spin_legend argument, which is currently enabled by default (I can change this).
  • Bug fix so that force_branches now produces identical branches for VASP's Line Mode versus an identical path generated by, e.g., sumo-kgen (i.e., one that doesn't have those pesky duplicate points). This doesn't affect the operation of sumo whatsoever since it is smart enough to get rid of the ghost k-points. However, this fix makes it possible to compare such band structures since they now have the same branches, and BSPlotter.add_bs won't throw an error.

I still need to work on the CLI implementation. I was thinking of something like:
* --code can now also accept a list of codes, which enables comparison mode.
* A --compare option forces comparison even if a single option is passed to --code (e.g., for comparing 3 different VASP band structures instead of trying to stitch them into one. For example: --filenames vasprun1.xml,vasprun2.xml --code vasp --compare).

Any feedback is appreciated! Here's an example band structure comparing a VASP calculation with 3 Quantum ESPRESSO calculations using different pseudopotentials (a little excessive, it's just meant to show you can do this).

image

There's also an example comparing DFT and W90 in my Wannier90 discussion issue.

@ajjackson
Copy link
Member

Clearly this is a useful plot for research work, and it would be good if they could be produced conveniently.

I do worry about the level of incompatibility between features: for example this will not play nicely with projected band structures. It will become very difficult to communicate which CLI options work together. (Already some of them are difficult to explain!) I wonder if instead this kind of plot would be a good case study for an improved Python API, and produced with user scripting.

If we did do this with the CLI I would drop --filenames and just allow --filename to take a variable number of arguments. --compare is also unnecessary: we can count the number of codes and filenames that were provided. If one code is specified we assume it applies to all filenames; if the lists are the same length we zip them together; otherwise we raise an error.

Perhaps the real answer is to separate orbital projected plots and line plots into separate CLI tools 🤔

@utf thoughts?

@utf
Copy link
Member

utf commented Jul 26, 2023

This looks like a great feature! Personally, I would be very happy to accept a PR.

Going on what @ajjackson said, I think allowing multiple options to --filename and --codes is the best way to go.

Regarding support for projected band structures, I would just raise an error in the command line if the user tries to specify both multiple filenames and projected/DOS plots at the same time.

@oashour
Copy link
Contributor Author

oashour commented Jul 27, 2023

Regarding projected band structures, I currently raise an exception if plotting multiple band structures from SBSPlotter.get_projected_plot.

I don't raise an exception when comparing multiple band structures and doing a DOS plot simultaneously, I personally don't see a problem with that (obviously as long as the source of the DOS is clearly explained somewhere in the paper), but I can raise an exception too if you'd prefer. Comparing DOS from multiple calculations doesn't quite make sense, so I won't be adding anything to sumo-dosplot.

For the CLI, note that --filename already accepts multiple values, so that sumo can stitch band structures together. If you pass multiple vasprun.xml files, there's no way to know if you want to compare them or stitch them. (I meant to use --filename not a new flag --filenames in my original PR, sorry for the confusion.)

I think the best course of action when multiple --filename values are passed:

  1. Single value of --code: stitch the band structures
  2. Multiple values of --code: zip filenames and codes, compare band structures.

The reason I thought of --compare is so you could pass a single code for multiple file names and compare them, i.e., --code vasp --compare is equivalent to --code vasp vasp vasp ... vasp. But I think it adds unnecessary complication, and it should be okay to ask the users to type a little extra to avoid confusion, I think?

I think the only thing missing for this feature is the CLI implementation, which shouldn't be too hard. I'll work on that when I get the chance and open a PR.

@ajjackson
Copy link
Member

ajjackson commented Jul 27, 2023

There is still some ambiguity if they want to compare stitched band structures 😞

@oashour
Copy link
Contributor Author

oashour commented Jul 28, 2023

I hadn't even thought about comparing stitched band structures... If we restrict the feature to comparing single-file band structures then the convention above would work fine.

But we could have situations where you want to compare two band structures where, e.g., the first 3 vasprun.xml files need to be stitched and compared to a second band structure coming from the fourth vasprun.xml file (no stitching). Or many other combinations.

Maybe we could use spaces to separate files that need to be stitched together and commas for files to be compared? Sort of like the --kpoints flag of sumo-kgen where "0 0 0, 0.5 0.5 0.5" means the two kpoints [0,0,0] and [0.5,0.5,0.5].

For example:

# current behavior, nothing changes, just stitch them
sumo-bandplot --code vasp --filenames vr1.xml vr2.xml 
# stitch first 2 files, compare to third file. All come from vasp.
sumo-bandplot --code vasp --filenames "vr1.xml vr2.xml, vr3.xml" 
# stitch vr{1,2}.xml coming from VASP, stitch pw{1,2}.xml coming from QE, 
# file 5 comes from wannier90, compare all 3 band structures
sumo-bandplot --code vasp espresso w90 --filenames "vr1.xml vr2.xml, pw1.xml pw2.xml, wannier_seed"

With this approach, we wouldn't need --code vasp vasp since it's no longer ambiguous whether you're trying to stitch or compare. I can't quite think of anything else at the moment 😅

@ajjackson
Copy link
Member

There is a lot of this ad-hoc syntax in Sumo already and it is getting increasingly difficult to document and explain which combinations of features are allowed.

I think the solution is probably to take a step back and stop trying to do so much with one CLI command. (That's not particularly a criticism of what you are going for here, it applies to bandplot and dosplot in general 😅 )

Perhaps there should be subcommands that each have a limited set of options. One feature of sumo-bandplot stitch would be to generate a band data file that is accepted as input to sumo-bandplot compare comparisons. Then if you try to specify orbital projections in a comparison you get told off by argparse and we don't need to write special logic for every illegal combination.

e.g.

sumo-bandplot stitch --code vasp --filename vr1.xml vr2.xml vr3.xml --to-json vasp-bands.json

sumo-bandplot compare --code json espresso w90 --filenames vasp-bands.json pw1.xml wannier_seed

@hongyi-zhao
Copy link

I am a bit doubtful whether there may always be corresponding CLI implementation for such complex and flexible (even higher) needs.

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

4 participants