-
Notifications
You must be signed in to change notification settings - Fork 13
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 NEBCalculator
with M3GNet and CHGNet example notebook
#13
Conversation
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.
Nice work @JiQi535, this looks great! 👍
I think we need to cut down on the amount of NEB_data
before merging as there are several MB of (unused?) traj data in this PR.
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.
I don't see you loading all these trajectory files in the test nor example notebook you added. Were they committed by accident?
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.
These can be deleted. They are truly not used anywhere. I will remove them from the PR.
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.
I have removed the traj files, and users can reproduce them with the notebook if needed. Now the NEB_data is below 1MB that majorly comes from the two png files in the notebook. Let me know if we need to futher downsize it or not.
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.
Could you replace the PNGs with vector graphics if those are smaller (PDF will be smaller but might not work in notebook, so could try SVG as well).
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.
I just tried and the pdf file (~2.5MB for each) extracted from VESTA would be >5 times larger than the png snapshot. Directly transferring the png snapshots to pdf version would not change the size much, e.g., from 284 to 276 KB. If the storage space is of concern, maybe we can just remove all NEB_data?
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.
Just use the PNG. Don't be silly. there are instances where vector formats are better and there are those where non-vector are better. this is one instance. Though 2.5 Mb is really nothing.
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.
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.
@janosh Thanks!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
===========================================
+ Coverage 99.11% 100.00% +0.88%
===========================================
Files 7 8 +1
Lines 227 271 +44
===========================================
+ Hits 225 271 +46
+ Misses 2 0 -2 ☔ View full report in Codecov by Sentry. |
if self.traj_folder is not None: | ||
os.makedirs(self.traj_folder, exist_ok=True) | ||
for i in range(len(self.images)): | ||
self.optimizer.attach( |
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.
Thanks @JiQi535, really nice contribution! |
NEBCalculator
with M3GNet and CHGNet example notebook
Summary
Added NEBCalc for NEB calculations with universal potentials. LiFePO4 is used as an example system, where M3GNet-MS, M3GNet-DIRECT, and CHGNet provide reasonable agreements to each other and to literature.
Major changes:
Checklist
ruff
.mypy
.duecredit
@due.dcite
decorators to reference relevant papers by DOI (example)