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

Fix sorting with MP sets #2587

Merged
merged 2 commits into from
Dec 30, 2024
Merged

Fix sorting with MP sets #2587

merged 2 commits into from
Dec 30, 2024

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Dec 30, 2024

Closes #2583, I think.

Copy link

codecov bot commented Dec 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.33%. Comparing base (e857e94) to head (16c8abb).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2587   +/-   ##
=======================================
  Coverage   97.33%   97.33%           
=======================================
  Files          85       85           
  Lines        3564     3566    +2     
=======================================
+ Hits         3469     3471    +2     
  Misses         95       95           

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

@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Dec 30, 2024

Pinging @zulissimeta: I believe this PR closes #2583. It certainly fixes a bug -- hopefully it's the same one you're seeing.

Previously, if we had Atoms("HOHMn"), the LDAUU flag would incorrectly be made as LDAUU = 0 0 0 3.9 when loading the MPRelaxSet. The reason this is incorrect is that the POSCAR generated from ASE will have only three element types at the top: H O Mn. Therefore, there will be a mismatch between the LDAUU entries and the corresponding elements any time that there is merging of elements into a single element type.

With this patch, the LDAUU flag would correctly be made as LDAUU = 0 0 3.9 since we generate the MP input set on the ASE-sorted structure so that it is ultimately written out in the correct way. This PR does not make any changes to a user's Atoms object --- I try not to mutate anything that might be user-facing.

If this does not solve your particular issue, please provide the sample Atoms object or POSCAR along with a minimal reproducible example of the exact issue so that I can troubleshoot it appropriately.

Note that this does not directly address the example you provided in #2583 because that is an ASE matter. If the user specifies Atoms("HOH") and ldauu=[1, 2, 3], the request is somewhat ill-defined because LDAUU is on a per-type basis, and it is generally expected that the POSCAR will be written with two (rather than three) atom types here: H and O. Ideally, there would be an option in the Vasp calculator to not sort or merge anything, in which case the ldauu=[1,2,3] request would then be viable. Atoms("OH") with ldauu=[1,2] is perfectly fine as-is; ASE does not re-sort this.

@Andrew-S-Rosen Andrew-S-Rosen enabled auto-merge (squash) December 30, 2024 05:30
@Andrew-S-Rosen Andrew-S-Rosen merged commit c3f9273 into main Dec 30, 2024
21 checks passed
@Andrew-S-Rosen Andrew-S-Rosen deleted the Andrew-S-Rosen-patch-3 branch December 30, 2024 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

MP GGA VASP workflows resort and cause problems with per-atom defaults like LDA+U
1 participant