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

WIP: Dos plots #123

Closed
wants to merge 3 commits into from
Closed

WIP: Dos plots #123

wants to merge 3 commits into from

Conversation

davSchnieders
Copy link

A naive implementation of a reader for DOSCAR.lobster files. Implements a function to plot DOS directly from the CLI. Connects to #41

Let me know if something needs changing, I know it is more or less hacked into the CLI, suggestions to make it better are very welcome.

@JaGeo
Copy link
Owner

JaGeo commented Jul 5, 2023

Thank you! I am tagging @ajjackson and @naik-aakash here as well as they probably also have some suggestions.

We are currently on a conference but start to work on LobsterPy again next week 😃

@JaGeo
Copy link
Owner

JaGeo commented Jul 5, 2023

I do have one question already: why did you decide to implement a new Dos class? Is there any functionality the pymatgen implementations are missing? We recently also improved the pymatgen plotter.

You can find an example on how to use the pymatgen implementations here: https://matgenb.materialsvirtuallab.org/2019/01/11/How-to-plot-and-evaluate-output-files-from-Lobster.html

(just also to make this clear again: anyone with a contribution to LobsterPy can be co-author on the paper. Just to make sure that you know that potential work on the implementation will be rewarded)

@davSchnieders davSchnieders changed the title Dos plots WIP: Dos plots Jul 5, 2023
@davSchnieders
Copy link
Author

The implementation in pymatgen has a dependency on the VASP POSCAR file, which was not needed for my purposes (and I am using Quantum Espresso anyway...). Hence,. I built my own DOSCAR parser, keeping it agnostic to the electronic structure code.

@davSchnieders
Copy link
Author

davSchnieders commented Jul 5, 2023

Anyway, I am now realizing that it is missing a way to treat spin-polarized systems, so I will add that in during tomorrows train ride!

(And I see a bunch of tests failing, something I will take care of as well)

@JaGeo
Copy link
Owner

JaGeo commented Jul 5, 2023

I see. We can certainly fix this directly in pymatgen as well. Tbh, I would prefer to simply extend the pymatgen version. It now also works with a Structure object. Thus, should be easy to adapt to QE. Could you maybe just provide an example case from QE? Structure input and DOSCAR.lobster? I will then see how to fix the pymatgen implementation.

Beyond this, we would also need tests.

@davSchnieders
Copy link
Author

I can understand your preference to use existing code, especially since LobsterPy already uses pymatgen and also has the POSCAR structure available, so the issues I had with the pymatgen DOSCAR parser do not apply here.

Tests: Yes, for sure, they are missing, as this originated from a small Python script, used solely by myself. I planned to add tests in the course of this pull request. However, since you opt for the pymatgen parser, I will close the pull request instead. In case you change your mind and want to use my parser instead, please let me know, and I will reopen the pull request and add the tests

@JaGeo
Copy link
Owner

JaGeo commented Jul 6, 2023

I would love to check if we can improve the pymatgen parser based on your implementation!

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

Successfully merging this pull request may close these issues.

2 participants