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

Add functionality to read ASE *.traj file in Trajectory class method from_file() #3422

Merged
merged 20 commits into from
Dec 23, 2023

Conversation

exenGT
Copy link
Contributor

@exenGT exenGT commented Oct 25, 2023

Summary:
Add functionality to read ASE *.traj file in Trajectory class method from_file(), located in core/trajectory.py.

Purpose:
As ASE *.traj file is a standard format for generating and storing relaxation / MD trajectories, it would be nice to have a method which can read a Pymatgen Trajectory instance from an ASE *.traj file.

@exenGT exenGT changed the title Added functionality to read ASE *.traj file in Trajectory class method from_file() Add functionality to read ASE *.traj file in Trajectory class method from_file() Oct 25, 2023
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.

We need a test for this. Could you upload an ASE traj file and read it in tests/core/test_trajectory.py?

pymatgen/core/trajectory.py Outdated Show resolved Hide resolved
@janosh janosh added enhancement A new feature or improvement to an existing one needs testing PRs that are not ready to merge due to lacking tests ase Atomic simulation environment labels Oct 25, 2023
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen left a comment

Choose a reason for hiding this comment

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

Some small comments.

pymatgen/core/trajectory.py Outdated Show resolved Hide resolved
constant_lattice=constant_lattice,
**kwargs,
)
if not is_mol:
Copy link
Member

Choose a reason for hiding this comment

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

You should do if is_mol (return from_molecules) and then else (return from_structures). Makes more sense to start with a positive logic statement rather than a negative.

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 @exenGT, almost ready to go! 👍

@@ -471,3 +471,7 @@ def test_xdatcar_write(self):
written_traj = Trajectory.from_file("traj_test_XDATCAR")
self._check_traj_equality(self.traj, written_traj)
os.remove("traj_test_XDATCAR")

def test_read_file(self):
Copy link
Member

Choose a reason for hiding this comment

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

best match the method name in the test name: i.e. test_from_file. Also, maybe check the traj length and composition of the 1st frame for extra safety.

@janosh janosh removed the needs testing PRs that are not ready to merge due to lacking tests label Nov 8, 2023
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 test is failing due to file not found. Also, could you compress tests/files/LiMnO2_chgnet_relax.traj?

@janosh janosh force-pushed the master branch 2 times, most recently from 3c23114 to 36e289c Compare December 19, 2023 02:10
@janosh janosh enabled auto-merge (squash) December 23, 2023 00:47
@janosh janosh merged commit c67739a into materialsproject:master Dec 23, 2023
22 checks passed
janosh added a commit to exenGT/pymatgen that referenced this pull request Dec 23, 2023
…from_file() (materialsproject#3422)

* Add functionality to read ASE *.traj files in Trajectory.from_file().

* fix Trajectory.from_file turning abs path into file name

---------

Signed-off-by: Jingyang Wang <[email protected]>
Signed-off-by: Janosh Riebesell <[email protected]>
Co-authored-by: Janosh Riebesell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ase Atomic simulation environment enhancement A new feature or improvement to an existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants