Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
improve ASE traj #633
improve ASE traj #633
Changes from all commits
820ca84
4594981
b055b88
bde8b7b
0377b27
da3daa8
70f7813
65d2797
42ff2b0
7f1a398
59e3247
a381f89
1585ff8
2fefd7f
793745c
76895a7
a12c374
0a77be5
ea85559
9dbcbd8
0b27552
c09894b
67e7876
2afdc4f
8771f8c
fe5163a
d41573b
17fcb16
e05c9ce
95939bd
52ef114
60212f6
57d938e
3c95376
c3c45ef
46171fc
1efef9b
b669ae1
33c5084
790d4b5
f0c5c7d
551920e
d7c98f9
e0b53f7
81f402a
9248833
8f4684d
eb66571
9893423
77e8ef4
02b4dac
45c8018
71d0e70
e3909e7
b49a4e2
4e0c083
0e22d30
f0febca
4a07590
a65144b
3eb842c
8918a90
6e2df4d
1a322ce
188f945
5d27076
1a466e3
6ffe27f
3ba5933
912d78c
aeb569c
9331a04
bb4d75d
c5fb9ba
0dadef3
b5a9171
a57f6d7
4c7b3ef
7f424fc
b4a9d05
aa96933
d1f02a3
bdcb887
1c1ebdc
ca474a9
b304fc9
7a0530b
1f8113e
14e48f8
7315d77
be33545
af1c94b
a222e8e
62fe922
bb0640f
77fb18a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.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.
Use
Generator
fromtyping
instead oftyping.Generator
.Python's typing module recommends using
Generator
directly for type hints. This aligns with modern Python practices and improves readability.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.
Update type annotations from
List
tolist
.Using
list
instead ofList
for type annotations is recommended as of Python 3.9 for better readability and alignment with modern Python practices.Also applies to: 162-162
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.
Ensure proper file handling in
to_system
andto_labeled_system
.The methods
to_system
andto_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
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.
Add unit tests for the new methods
to_system
andto_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
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.
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 ofNone
and then setting the value inside the method if it isNone
.Committable suggestion