Skip to content

Fix Ion.as_reduced_dict doesn't reduce charge#4514

Merged
shyuep merged 10 commits intomaterialsproject:masterfrom
DanielYang59:fix-ion-br-oxi-guess
Oct 8, 2025
Merged

Fix Ion.as_reduced_dict doesn't reduce charge#4514
shyuep merged 10 commits intomaterialsproject:masterfrom
DanielYang59:fix-ion-br-oxi-guess

Conversation

@DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Oct 7, 2025

Summary

  • Fix Ion.as_reduced_dict doesn't reduce charge
  • Replace seemingly unnecessary deepcopy in Ion.from_dict
  • Add missing tests
ion = Ion.from_dict({"Mn": 2, "O": 8, "charge": -2})
print(ion.as_reduced_dict()) 
- {'Mn': 1.0, 'O': 4.0, 'charge': -2}
+ {'Mn': 1.0, 'O': 4.0, 'charge': -1.0}

Args:
dct: {symbol: amount} dict.
"""
dct_copy = deepcopy(dct)
Copy link
Contributor Author

@DanielYang59 DanielYang59 Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think deepcopy is necessary here as values are immutable (added in c53d44b)? shallow copy should suffice to prevent the original dict being modified in place (charge still present), test added in 9d9a603

@DanielYang59 DanielYang59 marked this pull request as ready for review October 7, 2025 21:23
@DanielYang59 DanielYang59 marked this pull request as draft October 8, 2025 10:53
@DanielYang59 DanielYang59 marked this pull request as ready for review October 8, 2025 10:54
@shyuep shyuep merged commit f67f3bb into materialsproject:master Oct 8, 2025
44 checks passed
@DanielYang59 DanielYang59 deleted the fix-ion-br-oxi-guess branch October 8, 2025 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants