-
Notifications
You must be signed in to change notification settings - Fork 874
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
Generate SupercellTransformation from minimum boundary distance #3238
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3238 +/- ##
==========================================
- Coverage 74.62% 74.06% -0.57%
==========================================
Files 230 230
Lines 69403 69418 +15
Branches 16161 16165 +4
==========================================
- Hits 51794 51415 -379
- Misses 14533 14958 +425
+ Partials 3076 3045 -31
☔ View full report in Codecov by Sentry. |
Unit test added! |
Check if CubicSupercellTransformation already does what this does. If it doesn't, we need unit tests and documentation before accepting any PRs. |
@shyuep Thanks for the quick feedback.
To verify my guess, I have used Unit tests are already added, and I think documentation are also written in the PR. Please let me know if they are okay or if documentation should be also added elsewhere. Lastly, I believe this function can be useful not only for what we are doing, but also for phonon calculations in MatCal. @janosh While it might be less useful, as MatCal is not doing DFT, and cell size is not a major concern I guess. |
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.
@JiQi535 Looks great! I cleaned up the code and doc strings a little. Ready to go except would be nice to test more structures.
@JiQi535 I think CubicSupercell allows you to restrict max number of atoms. There is also value in having a "cubic-like" supercell. But I am fine with having this in SupercellTransform as well. But I would suggest you add some arg to enable limiting number of atoms. That is afterall what we really care about, not boundary distance. |
Summary
Added a method
from_boundary_distance
toSupercellTransformation
, which can automatically find aSupercellTransformation
that guarantee minimum distance periodic boundaries to be larger than a desired value (min_boundary_dist
). This method allows high throughput generation of supercells for all 154,718 MP structures within 5 minutes.Major feature
Instead of simply expand lattice parameters with fixed lattice angles, this method also provides the option to rotate lattice angle (
allow_rotation
) to find potential supercell satisfyingmin_boundary_dist
with a smaller number of atoms. Related evaluations have been done to all MP relaxed structures and listed below.Evaluation
All 154,718 MP ground-state structures are screened through with this implementation. As shown below, setting
allow_rotation=True
leads to noticeable decrease of supercell size for all threemin_boundary_dist
values of 6, 8, and 10 Å.Todos
min_boundary_dist
. Automatically identifying the smallest possible supercell seems a non-trivial task. Exhaustive enumeration needs more sophisticated scripts and more loops, while I expect the improvements to be small.