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

Updates for Vasprun with MD simulations #3489

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

gpetretto
Copy link
Contributor

Summary

I propose a few minor changes in the Vasprun object when parsing MD simulations and especially those produced with the ML option.

  • the converged_ionic property was False for an MD simulation. But the check did not seems reasonable for an MD
  • the number of steps for a simulation that involves ML MD is not given by the length of ionic_steps
  • in case of NPT simulation the parsing of stresses for ML MD simulations was missing

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.

The changes sound good to me! Thanks a lot

Copy link
Member

Choose a reason for hiding this comment

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

Can you compress?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, there's already tests/files/vasprun.xml.ml_md. Is that file incompatible with this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In principle the ml_md file will not cover the case of a simple MD, so for example it should miss this line:

return self.nionic_steps

I can remove it if you prefer saving space in the repository.

Copy link
Member

Choose a reason for hiding this comment

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

If the new file covers a superset of test cases, you can remove the old one and plug in the path to the new one instead (only 1 test needs updating).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, the new one will not cover the ML MD, that produces the md_data attribute. The data are partially overlapping, so will not cover the same set of lines. If I need to remove one, I think that removing the vasprun.xml.md would be better.

Copy link
Member

Choose a reason for hiding this comment

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

no need. where you load the file

vasp_run = Vasprun(f"{TEST_FILES_DIR}/vasprun.xml.md.gz")

could you add a brief comment what differentiates this file from vasprun.xml.ml_md to leave a paper trail of our discussion in the code.

@janosh janosh added enhancement A new feature or improvement to an existing one io Input/output functionality vasp Vienna Ab initio Simulation Package md Molecular dynamics labels Dec 1, 2023
@janosh janosh merged commit 5fb178f into materialsproject:master Dec 1, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement to an existing one io Input/output functionality md Molecular dynamics vasp Vienna Ab initio Simulation Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants