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 Composition for mixed species & element compositions #4256

Merged

Conversation

kavanase
Copy link
Contributor

Composition objects initialised as e.g. Composition({"Cd2+": 1, "Cd": 1, "Te2-": 2}) do not behave as expected, giving e.g. Composition.formula = "Cd3 Te2" instead of Cd2 Te2. This is due to Composition.__getitem__() matching all species and elements/non-species of a given input string; so that Composition["Cd"] here would return 2 (as desired), but than Composition.items() (which is use for get_el_amt_dict, formula and more) gives ("Cd2+", 1), ("Cd", 2), ("Te", 2) which is incorrect.
In the end, it's an easy fix to just define the items() method to avoid the Composition.__getitem__() (which is only needed when the user does Composition[...]).

Test added, which failed with previous code and passes now.

Context: Took a while to find what was going on here. Was originally flagged by this ValueError with ComputedStructureEntry: https://github.com/materialsproject/pymatgen/blob/master/src/pymatgen/entries/computed_entries.py#L588

With original pymatgen code:
image

image

With fixed code:
image

image

Flagged by tests in ShakeNBreak, because defect generation in pymatgen-analysis-defects mixes species with just element symbols; e.g. converting to just element symbol with get_element here: https://github.com/materialsproject/pymatgen-analysis-defects/blob/main/pymatgen/analysis/defects/core.py#L566-L576

pre-commit-ci bot and others added 4 commits January 17, 2025 10:36
@shyuep shyuep merged commit 7c0b65b into materialsproject:master Jan 17, 2025
43 checks passed
@shyuep
Copy link
Member

shyuep commented Jan 17, 2025

Great thanks.

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.

None yet

2 participants