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

Generalize fatband plots from Lobster #3688

Merged
merged 28 commits into from
Mar 14, 2024

Conversation

JaGeo
Copy link
Member

@JaGeo JaGeo commented Mar 13, 2024

Closes #3686

Todo:

  • More tests

@JaGeo JaGeo changed the title fatband fix Generalize fatband plots from Lobster Mar 13, 2024
@JaGeo
Copy link
Member Author

JaGeo commented Mar 13, 2024

@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.

self.structure = Structure.from_file(structure_file) # type: ignore
else:
self.structure = structure # type: ignore
self.lattice = self.structure.lattice.reciprocal_lattice # type: ignore
Copy link
Contributor

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")

self,
filenames: str | list = ".",
kpoints_file: str = "KPOINTS",
structure_file: str | None = "POSCAR.lobster",
Copy link
Member

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)

Copy link
Member Author

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.

Comment on lines 1300 to 1310
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,
)
Copy link
Member

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)

Copy link
Member Author

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!

Copy link
Member Author

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.

@DanielYang59
Copy link
Contributor

DanielYang59 commented Mar 14, 2024

Looks like the same download timeout error as #3675 from uv for large dependencies, see discussion here, maybe we should increase the UV_HTTP_TIMEOUT?

Caused by: Failed to download distribution due to network timeout. Try increasing UV_HTTP_TIMEOUT

@JaGeo
Copy link
Member Author

JaGeo commented Mar 14, 2024

@janosh weird failures during the dependency installations

@janosh
Copy link
Member

janosh commented Mar 14, 2024

no point in increasing UV_HTTP_TIMEOUT since it's already very high (300s). hopefully, uv comes up with a fix soon-ish else we might have to pre-install pytorch with pip before calling uv

@janosh janosh added enhancement A new feature or improvement to an existing one data viz PRs and issues about pymatgen plotting functionality labels Mar 14, 2024
@DanielYang59
Copy link
Contributor

we might have to pre-install pytorch with pip before calling uv

This sounds like a good temporary fix 😃

@JaGeo
Copy link
Member Author

JaGeo commented Mar 14, 2024

@janosh are the changes fine? Tests have run through after I triggered them again

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @JaGeo, lgtm. 👍

@janosh janosh enabled auto-merge (squash) March 14, 2024 15:56
@janosh
Copy link
Member

janosh commented Mar 14, 2024

Todo:
More tests

is the todo resolved?

@JaGeo
Copy link
Member Author

JaGeo commented Mar 14, 2024

Todo:
More tests

is the todo resolved?

Yes, i added several new tests.

@janosh janosh disabled auto-merge March 14, 2024 17:08
@janosh janosh merged commit 72977a9 into materialsproject:master Mar 14, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data viz PRs and issues about pymatgen plotting functionality enhancement A new feature or improvement to an existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fatband plot with lobster: not only vasp should be supported
3 participants