-
Notifications
You must be signed in to change notification settings - Fork 868
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
Generalize fatband plots from Lobster #3688
Conversation
@janosh I had to fight with mypy for the Structure. I did not see why it complained. Maybe, you see something... I added ignores for now. |
pymatgen/io/lobster/outputs.py
Outdated
self.structure = Structure.from_file(structure_file) # type: ignore | ||
else: | ||
self.structure = structure # type: ignore | ||
self.lattice = self.structure.lattice.reciprocal_lattice # type: ignore |
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.
I wish you don't mind me pop in, try this:
warnings.warn("Make sure all relevant FATBAND files were generated and read in!")
warnings.warn("Use Lobster 3.2.0 or newer for fatband calculations!")
if vasprun_file is None and efermi is None:
raise ValueError("vasprun_file or efermi have to be provided")
if structure_file is not None:
self.structure = Structure.from_file(structure_file)
elif structure is not None:
self.structure = structure
else:
raise ValueError("structure_file or structure have to be provided")
pymatgen/io/lobster/outputs.py
Outdated
self, | ||
filenames: str | list = ".", | ||
kpoints_file: str = "KPOINTS", | ||
structure_file: str | None = "POSCAR.lobster", |
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.
following Python's "there should be one obvious way to do something" i'd advocate for removing the structure_file
keyword. afaict, it's only saving the user 1 line
Structure.from_file(structure_file)
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.
Removed the structure_file
then.
tests/io/lobster/test_inputs.py
Outdated
filename=f"{TEST_FILES_DIR}/cohp/Fatband_SiO2/Test_p_x/vasprun.xml", | ||
ionic_step_skip=None, | ||
ionic_step_offset=0, | ||
parse_dos=True, | ||
parse_eigen=False, | ||
parse_projected_eigen=False, | ||
parse_potcar_file=False, | ||
occu_tol=1e-8, | ||
exception_on_bad_xml=True, | ||
).final_structure, | ||
) |
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.
maybe extract repeated kwargs into a separate dict to reuse in each Vasprun
call:
kwargs = dict(
ionic_step_skip=None,
ionic_step_offset=0,
parse_dos=True,
parse_eigen=False,
parse_projected_eigen=False,
parse_potcar_file=False,
occu_tol=1e-8,
exception_on_bad_xml=True,
# ...
)
Vasprun(filenames="...", **kwargs)
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.
I decided to rather define a structure in the setup and then reuse that. But I also got rid of lot of repetitions. Thanks for the suggestion!
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.
Not sure why I did not do this in the first place. I guess I was too distracted with getting it to work in general.
@janosh weird failures during the dependency installations |
no point in increasing |
This sounds like a good temporary fix 😃 |
@janosh are the changes fine? Tests have run through after I triggered them again |
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 @JaGeo, lgtm. 👍
is the todo resolved? |
Yes, i added several new tests. |
Closes #3686
Todo: