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 apply_operation(fractional=True) #4057

Merged
merged 3 commits into from
Sep 9, 2024
Merged

Fix apply_operation(fractional=True) #4057

merged 3 commits into from
Sep 9, 2024

Conversation

kavanase
Copy link
Contributor

@kavanase kavanase commented Sep 9, 2024

Summary

Structure.apply_operation() fails when using the fractional=True operation.
The issue is that the SymmOp is applied to the site frac coords (in the original lattice), but then these new frac coords (in the old basis) are used directly with the new (transformed) lattice, giving a 'doubled' transformation and messing with the output.

Fix added here, matching the implementation in doped (which also has a couple more options and a cleaner implementation).

MWE:
image
image

(Notebook and structure being used attached too as always 😉 , with more examples)

Pymatgen_SymmOp_Fix_PR.zip

@kavanase
Copy link
Contributor Author

kavanase commented Sep 9, 2024

This fixed implementation is tested and shown to be working in the attached notebook as well

kavanase added a commit to SMTG-Bham/doped that referenced this pull request Sep 9, 2024
@janosh janosh added needs testing PRs that are not ready to merge due to lacking tests fix Bug fix PRs symmetry Space groups and the like core Pymatgen core labels Sep 9, 2024
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.

thanks a lot for the fix @kavanase. could you add a unit test that covers this change?

@kavanase
Copy link
Contributor Author

kavanase commented Sep 9, 2024

Done!
Also checked that this test fails with the old code (for fractional=True), but works all fine with the new code.

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.

thanks, good stuff!

@janosh janosh removed the needs testing PRs that are not ready to merge due to lacking tests label Sep 9, 2024
@janosh janosh merged commit ed4de1d into materialsproject:master Sep 9, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Pymatgen core fix Bug fix PRs symmetry Space groups and the like
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants