SMTG-Bham / sumo

Heavyweight plotting tools for ab initio calculations
https://smtg-bham.github.io/sumo/
MIT License
205 stars 81 forks source link

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

Open oashour opened 1 year ago

oashour commented 1 year ago

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:

I still need to work on the CLI implementation. I was thinking of something like:

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 commented 1 year ago

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 :thinking:

@utf thoughts?

utf commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

oashour commented 1 year ago

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 commented 1 year ago

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 :sweat_smile: )

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 commented 1 year ago

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