improve ASE traj #633

thangckt · 2024-04-04T15:32:02Z

add functions to convert from others formats to ASE traj format

Summary by CodeRabbit

New Features
- Enhanced system and labeled system handling with new parameters and functionalities.
- Improved stress calculations by modifying method parameters.
Bug Fixes
- Corrected return types for several methods to ensure consistency and reliability.
Tests
- Added new test cases to validate system setups and trajectory file operations.
Chores
- Updated Python version matrix in workflow to include only version 3.11.

for more information, see https://pre-commit.ci

…ha_devel

for more information, see https://pre-commit.ci

…ha_devel

for more information, see https://pre-commit.ci

…ha_devel

coderabbitai · 2024-05-27T12:36:18Z

Walkthrough

The recent changes enhance the functionality of the dpdata/plugins/ase.py module by updating the return types of several methods and adding new parameters for improved flexibility. Additionally, the Python version matrix for testing has been streamlined, and new test classes have been introduced to ensure the robustness of the changes.

Changes

File	Change Summary
`dpdata/plugins/ase.py`	Updated return types, added new parameters, and modified functionality in several methods.
`.github/workflows/test.yml`	Updated Python version matrix to include only version "3.11".
`tests/test_ase_traj.py`	Added new test classes `TestASEtraj4` and `TestASEtraj4Labeled` to test trajectory-related operations.

Sequence Diagram(s) (Beta)

sequenceDiagram
    participant User
    participant ASEPlugin
    participant ASEIO
    participant TestSuite

    User->>ASEPlugin: Call to_system(data, file_name="confs.traj")
    ASEPlugin->>ASEIO: Save data to trajectory file
    ASEIO-->>ASEPlugin: Trajectory file saved
    ASEPlugin-->>User: Return List[ase.Atoms]

    User->>ASEPlugin: Call to_labeled_system(data, file_name="labeled_confs.traj")
    ASEPlugin->>ASEIO: Save labeled data to trajectory file
    ASEIO-->>ASEPlugin: Labeled trajectory file saved
    ASEPlugin-->>User: Return List[ase.Atoms]

    TestSuite->>ASEPlugin: Run TestASEtraj4
    ASEPlugin-->>TestSuite: Test results
    TestSuite->>ASEPlugin: Run TestASEtraj4Labeled
    ASEPlugin-->>TestSuite: Test results

Tip

Early Access Features

gpt-4o model for chat

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to full the review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

for more information, see https://pre-commit.ci

codspeed-hq · 2024-05-27T12:38:19Z

CodSpeed Performance Report

Merging #633 will not alter performance

_{Comparing thangckt:PR (bb0640f) with devel (c5b36bb)}

Summary

✅ 2 untouched benchmarks

coderabbitai

Actionable comments posted: 6

coderabbitai · 2024-05-27T12:40:06Z

dpdata/plugins/ase.py

@@ -1,6 +1,6 @@
 from __future__ import annotations

-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Generator, List


Consider using specific imports instead of TYPE_CHECKING.

Using specific imports can improve clarity and reduce the overhead of unnecessary imports when they are not needed for type checking.

coderabbitai · 2024-05-27T12:40:06Z

dpdata/plugins/ase.py

@@ -10,6 +10,7 @@

 if TYPE_CHECKING:
    import ase
+    from ase.io import Trajectory


Move import ase.io.Trajectory out of type-checking block.

This import is used in the runtime code, not just for type hints. It should be placed outside the if TYPE_CHECKING: block to ensure it is always imported.

coderabbitai · 2024-05-27T12:40:06Z

dpdata/plugins/ase.py

@@ -110,7 +111,7 @@
        step: int | None = None,
        ase_fmt: str | None = None,
        **kwargs,
-    ) -> ase.Atoms:
+    ) -> Generator[ase.Atoms, None, None]:


Use Generator from typing instead of typing.Generator.

Python's typing module recommends using Generator directly for type hints. This aligns with modern Python practices and improves readability.

coderabbitai · 2024-05-27T12:40:06Z

dpdata/plugins/ase.py

@@ -140,7 +141,7 @@
        frames = ase.io.read(file_name, format=ase_fmt, index=slice(begin, end, step))
        yield from frames

-    def to_system(self, data, **kwargs):
+    def to_system(self, data, **kwargs) -> List[ase.Atoms]:


Update type annotations from List to list.

- def to_system(self, data, **kwargs) -> List[ase.Atoms]: + def to_system(self, data, **kwargs) -> list[ase.Atoms]: - def to_labeled_system(self, data, *args, **kwargs) -> List[ase.Atoms]: + def to_labeled_system(self, data, *args, **kwargs) -> list[ase.Atoms]:

Using list instead of List for type annotations is recommended as of Python 3.9 for better readability and alignment with modern Python practices.

Also applies to: 162-162

Committable suggestion was skipped due low confidence.

for more information, see https://pre-commit.ci

coderabbitai

Actionable comments posted: 2

coderabbitai · 2024-05-27T14:29:41Z

tests/test_ase_traj.py

+@unittest.skipIf(skip_ase, "skip ase related test. install ase to fix")
+class TestASEtraj4(unittest.TestCase, CompSys, IsPBC):
+    def setUp(self):
+        system_1 = dpdata.System("ase_traj/MoS2", fmt="deepmd")
+        system_1.to(file_name="tmp.traj", fmt="ase/traj")
+        self.system_2 = dpdata.System("tmp.traj", fmt="ase/traj")
+        self.system_1 = system_1
+        self.places = 6
+        self.e_places = 6
+        self.f_places = 6
+        self.v_places = 4


Add test methods to TestASEtraj4.

This test class sets up systems but does not include any assertions to verify the correctness of operations. Would you like me to help by adding some example test methods?

coderabbitai · 2024-05-27T14:29:41Z

tests/test_ase_traj.py

+@unittest.skipIf(skip_ase, "skip ase related test. install ase to fix")
+class TestASEtraj4Labeled(unittest.TestCase, CompLabeledSys, IsPBC):
+    def setUp(self):
+        system_1 = dpdata.LabeledSystem("ase_traj/MoS2", fmt="deepmd")
+        system_1.to(file_name="tmp1.traj", fmt="ase/traj")
+        self.system_2 = dpdata.LabeledSystem("tmp1.traj", fmt="ase/traj")
+        self.system_1 = system_1
+        self.places = 6
+        self.e_places = 6
+        self.f_places = 6
+        self.v_places = 4


Add test methods to TestASEtraj4Labeled.

Like TestASEtraj4, this class also prepares systems but lacks assertions to check the outcomes. I can assist in creating some test methods if you need.

for more information, see https://pre-commit.ci

Signed-off-by: C. Thang Nguyen <46436648+thangckt@users.noreply.github.com>

dpdata/plugins/ase.py

for more information, see https://pre-commit.ci

coderabbitai

Actionable comments posted: 5

coderabbitai · 2024-05-29T00:43:01Z

dpdata/plugins/ase.py

+        list_atoms = ASEStructureFormat().to_system(data, **kwargs)
+        traj = Trajectory(file_name, "a")
+        _ = [traj.write(atom) for atom in list_atoms]
+        traj.close()
+        return


Add unit tests for the new methods to_system and to_labeled_system.

These methods are critical for the functionality added in this PR but are not covered by tests according to the static analysis. Would you like assistance in creating these tests?

Also applies to: 328-332

coderabbitai · 2024-05-29T00:43:01Z

dpdata/plugins/ase.py

@@ -94,7 +95,7 @@ def from_labeled_system(self, atoms: ase.Atoms, **kwargs) -> dict:
            "forces": np.array([forces]),
        }
        try:
-            stress = atoms.get_stress(False)
+            stress = atoms.get_stress(voigt=False)


Consider handling PropertyNotImplementedError more explicitly.

While the current implementation passes silently if PropertyNotImplementedError is raised, it might be beneficial to log this occurrence or handle it in a way that does not silently ignore potential issues. Consider adding logging or another form of error handling.

coderabbitai · 2024-05-29T00:43:01Z

dpdata/plugins/ase.py

@@ -300,6 +301,46 @@ def from_labeled_system(

        return dict_frames

+    def to_system(self, data, file_name: str = "confs.traj", **kwargs) -> None:


Ensure proper file handling in to_system and to_labeled_system.

The methods to_system and to_labeled_system perform file operations such as checking if a file exists and removing it. It's crucial to ensure that these operations are safe and do not lead to unintended data loss. Consider adding checks or warnings to inform the user about the file deletion.

Also applies to: 323-323

coderabbitai · 2024-05-29T00:43:01Z

dpdata/plugins/ase.py

+        _ = [traj.write(atom) for atom in list_atoms]
+        traj.close()
+        return
+


Review the use of default parameters in ASEMinimizer.

The constructor of ASEMinimizer uses mutable default arguments (optimizer_kwargs). This is a common Python pitfall, as default mutable arguments can lead to unexpected behavior if the object is modified. Consider using a default value of None and then setting the value inside the method if it is None.

- optimizer_kwargs: dict = {} + optimizer_kwargs: dict | None = None if optimizer_kwargs is None: optimizer_kwargs = {}

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change

optimizer_kwargs: dict | None = None

if optimizer_kwargs is None:

optimizer_kwargs = {}

coderabbitai · 2024-05-29T00:43:01Z

dpdata/plugins/ase.py

@@ -140,7 +141,7 @@ def from_multi_systems(
        frames = ase.io.read(file_name, format=ase_fmt, index=slice(begin, end, step))
        yield from frames

-    def to_system(self, data, **kwargs):
+    def to_system(self, data, **kwargs) -> list[ase.Atoms]:


Update type annotations from List to list.

- def to_system(self, data, **kwargs) -> List[ase.Atoms]: + def to_system(self, data, **kwargs) -> list[ase.Atoms]: - def to_labeled_system(self, data, *args, **kwargs) -> List[ase.Atoms]: + def to_labeled_system(self, data, *args, **kwargs) -> list[ase.Atoms]:

Using list instead of List for type annotations is recommended as of Python 3.9 for better readability and alignment with modern Python practices.

Also applies to: 162-162

Committable suggestion was skipped due low confidence.

thangckt and others added 30 commits March 13, 2024 17:43

add ASE's traj

820ca84

Update ase.py

4594981

Update ase.py

b055b88

Update ase.py

bde8b7b

Update ase.py

0377b27

finish add ASE's traj support

da3daa8

[pre-commit.ci] auto fixes from pre-commit.com hooks

70f7813

for more information, see https://pre-commit.ci

Update ase.py

65d2797

Merge branch 'tha_devel' of https://github.com/thangckt/dpdata into t…

42ff2b0

…ha_devel

[pre-commit.ci] auto fixes from pre-commit.com hooks

7f1a398

for more information, see https://pre-commit.ci

Update ase.py

59e3247

[pre-commit.ci] auto fixes from pre-commit.com hooks

a381f89

for more information, see https://pre-commit.ci

Update ase.py

1585ff8

[pre-commit.ci] auto fixes from pre-commit.com hooks

2fefd7f

for more information, see https://pre-commit.ci

Update ase.py

793745c

[pre-commit.ci] auto fixes from pre-commit.com hooks

76895a7

for more information, see https://pre-commit.ci

Update ase.py

a12c374

Update test_ase_traj.py

0a77be5

u

ea85559

[pre-commit.ci] auto fixes from pre-commit.com hooks

9dbcbd8

for more information, see https://pre-commit.ci

Update test_ase_traj.py

0b27552

Merge branch 'tha_devel' of https://github.com/thangckt/dpdata into t…

c09894b

…ha_devel

[pre-commit.ci] auto fixes from pre-commit.com hooks

67e7876

for more information, see https://pre-commit.ci

up

2afdc4f

u

8771f8c

[pre-commit.ci] auto fixes from pre-commit.com hooks

fe5163a

for more information, see https://pre-commit.ci

Update test_ase_traj.py

d41573b

Merge branch 'tha_devel' of https://github.com/thangckt/dpdata into t…

17fcb16

…ha_devel

u

e05c9ce

Update ase.py

95939bd

Merge remote-tracking branch 'upstream/devel' into PR

a57f6d7

[pre-commit.ci] auto fixes from pre-commit.com hooks

4c7b3ef

for more information, see https://pre-commit.ci

coderabbitai bot reviewed May 27, 2024

View reviewed changes

thangckt and others added 12 commits May 27, 2024 21:50

Update test_ase_traj.py

7f424fc

Merge branch 'PR' of https://github.com/thangckt/dpdata into PR

b4a9d05

Update test_ase_traj.py

aa96933

Update test_ase_traj.py

d1f02a3

Update test_ase_traj.py

bdcb887

Update test_ase_traj.py

1c1ebdc

[pre-commit.ci] auto fixes from pre-commit.com hooks

ca474a9

for more information, see https://pre-commit.ci

Update ase.py

b304fc9

u

7a0530b

u

1f8113e

Update test.yml

14e48f8

u

7315d77

coderabbitai bot reviewed May 27, 2024

View reviewed changes

thangckt and others added 3 commits May 27, 2024 23:31

u

be33545

[pre-commit.ci] auto fixes from pre-commit.com hooks

af1c94b

for more information, see https://pre-commit.ci

Update test.yml

a222e8e

thangckt requested a review from njzjz May 27, 2024 14:38

Merge branch 'devel' into PR

62fe922

Signed-off-by: C. Thang Nguyen <46436648+thangckt@users.noreply.github.com>

njzjz requested changes May 28, 2024

View reviewed changes

dpdata/plugins/ase.py Outdated Show resolved Hide resolved

thangckt and others added 2 commits May 29, 2024 09:32

Update ase.py

bb0640f

[pre-commit.ci] auto fixes from pre-commit.com hooks

77fb18a

for more information, see https://pre-commit.ci

thangckt requested a review from njzjz May 29, 2024 00:35

coderabbitai bot reviewed May 29, 2024

View reviewed changes

njzjz approved these changes May 29, 2024

View reviewed changes

wanghan-iapcm merged commit 199afc1 into deepmodeling:devel May 29, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve ASE traj #633

improve ASE traj #633

thangckt commented Apr 4, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented May 27, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

codspeed-hq bot commented May 27, 2024 •

edited

Loading

coderabbitai bot left a comment

coderabbitai bot May 27, 2024

coderabbitai bot May 27, 2024

coderabbitai bot May 27, 2024

coderabbitai bot May 27, 2024

coderabbitai bot left a comment

coderabbitai bot May 27, 2024

coderabbitai bot May 27, 2024

coderabbitai bot left a comment

coderabbitai bot May 29, 2024

coderabbitai bot May 29, 2024

coderabbitai bot May 29, 2024

coderabbitai bot May 29, 2024

coderabbitai bot May 29, 2024

		@@ -300,6 +301,46 @@ def from_labeled_system(

		return dict_frames

		def to_system(self, data, file_name: str = "confs.traj", **kwargs) -> None:

+  optimizer_kwargs: dict | None = None
+  if optimizer_kwargs is None:
+      optimizer_kwargs = {}

improve ASE traj #633

improve ASE traj #633

Conversation

thangckt commented Apr 4, 2024 • edited by coderabbitai bot Loading

Summary by CodeRabbit

coderabbitai bot commented May 27, 2024 • edited Loading

Walkthrough

Changes

Sequence Diagram(s) (Beta)

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

codspeed-hq bot commented May 27, 2024 • edited Loading

CodSpeed Performance Report

Merging #633 will not alter performance

Summary

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot May 27, 2024

Choose a reason for hiding this comment

coderabbitai bot May 27, 2024

Choose a reason for hiding this comment

coderabbitai bot May 27, 2024

Choose a reason for hiding this comment

coderabbitai bot May 27, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot May 27, 2024

Choose a reason for hiding this comment

coderabbitai bot May 27, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot May 29, 2024

Choose a reason for hiding this comment

coderabbitai bot May 29, 2024

Choose a reason for hiding this comment

coderabbitai bot May 29, 2024

Choose a reason for hiding this comment

coderabbitai bot May 29, 2024

Choose a reason for hiding this comment

coderabbitai bot May 29, 2024

Choose a reason for hiding this comment

thangckt commented Apr 4, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented May 27, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

codspeed-hq bot commented May 27, 2024 •

edited

Loading