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

Generate SupercellTransformation from minimum boundary distance #3238

Merged
merged 15 commits into from Aug 14, 2023

Conversation

JiQi535
Copy link
Contributor

@JiQi535 JiQi535 commented Aug 13, 2023

Summary

Added a method from_boundary_distance to SupercellTransformation, which can automatically find a SupercellTransformation 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 satisfying min_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 three min_boundary_dist values of 6, 8, and 10 Å.
Screenshot 2023-08-13 at 6 58 51 PM
Screenshot 2023-08-13 at 6 59 33 PM
Screenshot 2023-08-13 at 7 06 51 PM

Todos

  1. Unittests are not yet added.
  2. Though this implementation tries to find smaller supercell with some lattice rotation, it does not guarantee to find the smallest possible supercell satisfying 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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2023

Codecov Report

Patch coverage: 96.87% and project coverage change: -0.57% ⚠️

Comparison is base (d9fdfef) 74.62% compared to head (485ce21) 74.06%.
Report is 2 commits behind head on master.

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     
Files Changed Coverage Δ
...matgen/transformations/standard_transformations.py 86.49% <91.66%> (+0.16%) ⬆️
pymatgen/core/lattice.py 93.77% <100.00%> (+0.03%) ⬆️
pymatgen/core/structure.py 85.77% <100.00%> (ø)
pymatgen/io/lammps/data.py 96.96% <100.00%> (ø)

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@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 labels Aug 13, 2023
@JiQi535
Copy link
Contributor Author

JiQi535 commented Aug 14, 2023

Unit test added!

@shyuep
Copy link
Member

shyuep commented Aug 14, 2023

Check if CubicSupercellTransformation already does what this does. If it doesn't, we need unit tests and documentation before accepting any PRs.

@JiQi535
Copy link
Contributor Author

JiQi535 commented Aug 14, 2023

@shyuep Thanks for the quick feedback.

CubicSupercellTransformation can achieve supercells satisfying desired min_length, but it likely produce larger than needed supercells, as its "iteratively increase the size of the supercell until the largest inscribed cube’s side length is at least min_length". (quoted from pymatgen documentations) The from_boundary_distance method does not look for a supercell that can contain a cube inside it, instead, it directly focus on minimum boundary distance, so it is able to find relatively small supercells.

To verify my guess, I have used CubicSupercellTransformation (cubic_trafo) to generate supercells of all 154,718 MP relaxed structures. I have set min_length=8, inline with min_boundary_dist=8 for from_boundary_distance. And I have used force_diagonal=False and force_90_degrees=False, so that cubic_trafo should produce smallest possible supercell from its implementation. As in below plot, CubicSupercellTransformation generated supercells are in average over 1 time larger than supercells generated with from_boundary_distance.
Screenshot 2023-08-14 at 10 05 49 AM

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.

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.

@JiQi535 Looks great! I cleaned up the code and doc strings a little. Ready to go except would be nice to test more structures.

@shyuep
Copy link
Member

shyuep commented Aug 14, 2023

@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.

@janosh janosh merged commit d094726 into materialsproject:master Aug 14, 2023
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement to an existing one needs testing PRs that are not ready to merge due to lacking tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants