-
Notifications
You must be signed in to change notification settings - Fork 12
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
Soc PROCAR
handling and more
#28
Conversation
… in the field (otherwise most will skip to the examples)
… as discussed in #10. Tested and works as expected
…ge accordingly. Tested and works as expected
…ich atoms to project (rather than `atoms_idx`), by reading from POSCAR/CONTCAR. Tested and works as expected. Avoids using `pymatgen` as a dependency also. Will update examples to show this functionality
… do the same thing just in different plotting functions (which depends on whether `--combined` is used). Tested and all works as expected
Hi @zhubonan! But if I swap the colour choice, so that I dug around for a long while trying to find the origin of this, and how it could be improved. Basically it's related to how the interpolation is done in the transformed LAB space, which can result in some non-intuitive behaviour like this, depending on the colours being mixed (as you end up interpolating into the 'white' origin centre in LAB). I checked and this is similarly the case in
So I did a systematic check through the different possible colourspaces, using either red-green-blue or pastel red-green-blue as the basis, with these weight choices (covering the typical mixing spectrum)
Using the red-green-blue basis: My conclusions/opinions would be that (1) pastel colours are nicer, (2) RGB/XYZ/LAB make intuitive sense mostly, so suggest sticking with LAB as the default colourspace, using the pastel colours if the number of projected species is <= 3 (else red, green, blue, purple, orange, yellow as suggested by colorbrewer.org) and (3) making colorspace an optional parameter at the CLI level. |
Jupyter notebook for testing and plotting these colours here if useful: |
Fantastic! Thanks for making such a detailed investigation and tweaking. I am happy with the changes. |
Ok great! I'll update the docs to match the new defaults, and add the 'example outputs' mini section discussed offline, and then this is ready to be merged ✌️ 🏎️ |
All final bits discussed now implemented @zhubonan, this is ready to be merged! 🚀 |
Some last formatting changes added there @zhubonan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements!
Just one minor comment regarding closing file object in _read
- I think with
clause if not needed. If Procar
receives file objects (can be streams) rather than file names/paths, we should keep these objects open .
Also, now |
863a4ae
to
ebf053e
Compare
Hi @zhubonan! About saving the That said, something related I was going to mention, I think the loops in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good!
Good point 👍 , can probably implement it later! Yeah , I agree there is not much point to save PROCAR in JSON for now. |
Merged! Thanks again for fixing the PROCAR SOC bug and adding secondary axis DOS plot 🎊 |
This is mainly a fix to update the
PROCAR
handling ineasyunfold
(also updating to make theProcar
object JSON serializable), and in particular updating to handle SOC (ncl)PROCAR
s. This is now dynamically determined within theProcar
parsing code, so when plotting via the CLI the user doesn't need to specify--ncl
(previously this information wasn't passed on to thePROCAR
parsing anyway).This fixes the cases I found where plotting failed due to crashes when trying to parse SOC
PROCAR
s, now fixed, giving some nice plots like this:I've also checked through with previous example cases (NaBiS$_2$, MgO) to confirm the code is working as expected.
This also implements some other fixes:
sumo
DOS plots.sumo
orbital colour settings if atom projections aren't used and the user doesn't specifyvscale