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 Procar.get_projection_on_elements for structures with multiple same-element ionic sites #3261

Merged
merged 4 commits into from
Aug 23, 2023

Conversation

Na-Kawa
Copy link
Contributor

@Na-Kawa Na-Kawa commented Aug 22, 2023

Summary

Major changes:
Fixes a bug in the get_projection_on_elements function of Procar. In the current version, the code computes projections only for the final ion of each element when there are multiple ionic sites of the same element in the structure. To ensure projections are correctly computed and accumulated for all ions, I have corrected the assignment operator from '=' to '+='."

Signed-off-by: Naoto kawaguchi <[email protected]>
@janosh janosh added needs testing PRs that are not ready to merge due to lacking tests io Input/output functionality fix Bug fix PRs vasp Vienna Ab initio Simulation Package labels Aug 22, 2023
@Na-Kawa Na-Kawa changed the title Update outputs.py Bugfix for Procar get_projection_on_elements Aug 22, 2023
@Na-Kawa Na-Kawa changed the title Bugfix for Procar get_projection_on_elements Bugfix for Procar get_projection_on_elements function Aug 22, 2023
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 for fixing this @Na-Kawa! 👍 Could you add a test that makes sure get_projection_on_elements() now works correctly for multiple ionic sites?

Add test for get_projction_on_elements() when structure have multiple ionic sites with the same elements.

Signed-off-by: Naoto kawaguchi <[email protected]>
@Na-Kawa
Copy link
Contributor Author

Na-Kawa commented Aug 23, 2023

Hello @janosh,
I have added the test code for get_projction_on_elements() when the structure have multiple ionic sites with the same elements. Please check if it meets the requirements.

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.

Looks great! Thanks for the test! 👍

@janosh janosh changed the title Bugfix for Procar get_projection_on_elements function Fix Procar.get_projection_on_elements for structures with multiple same-element ionic sites Aug 23, 2023
@janosh janosh merged commit 6dacfff into materialsproject:master Aug 23, 2023
@janosh janosh removed the needs testing PRs that are not ready to merge due to lacking tests label Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PRs io Input/output functionality vasp Vienna Ab initio Simulation Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants