Skip to content

fix: keep Pourbaix entries whose composition equals comp_dict - #4714

Open
raashish1601 wants to merge 2 commits into
materialsproject:mainfrom
raashish1601:fix/4709-pourbaix-single-entry
Open

raashish1601 wants to merge 2 commits into
materialsproject:mainfrom
raashish1601:fix/4709-pourbaix-single-entry

Conversation

@raashish1601

Copy link
Copy Markdown

Fixes #4709

PourbaixDiagram.process_multientry builds Reaction(entry_comps + [H, O], [prod_comp]) and read the product coefficient with rxn.get_coeff(prod_comp). get_coeff looks the composition up in all_comp and returns the first match, so when an entry has the same composition as the product (e.g. Cu3P with comp_dict={"Cu": 3, "P": 1}) it returns the reactant coefficient (-1). The positive-coefficient check then fails and the entry is never turned into a MultiEntry, so the compound can't be a stable domain of its own diagram and its decomposition energy comes out negative.

The product is always the last composition in the reaction, so this takes rxn.coeffs[-1] instead.

With the reproducer from the issue, Cu3P(s) is now a stable domain and its decomposition energy at pH 7, -1 V is 0 (was -0.25 eV/atom). Added test_entry_with_comp_dict_composition, which fails on master and passes with the change. tests/analysis/test_pourbaix_diagram.py passes locally (22 passed), ruff and ruff format are clean, and mypy reports the same pre-existing errors as master for this file.

process_multientry read the product coefficient with rxn.get_coeff(prod_comp). When an entry has the same composition as the product, get_coeff finds the reactant first and returns -1, so the entry was never turned into a MultiEntry and could not be a stable domain of its own diagram. Take the product coefficient from the last position instead.

Fixes materialsproject#4709
@shyuep

shyuep commented Oct 7, 2026

Copy link
Copy Markdown
Member

Automated PR review generated by Claude (posted via @shyuep's account); sanity-check before acting.

Good: The root cause is identified correctly. rxn.get_coeff(prod_comp) looks up by composition, so an entry whose composition equals comp_dict returns the reactant's coefficient. That makes the weights invalid and drops the entry. The regression test checks both process_multientry and the end-to-end stable_entries / decomposition energy.

Substantive:

  • No CI has run (zero status checks on the branch). Treat this as untested until the workflow is approved or triggered.
  • rxn.coeffs[-1] assumes Reaction keeps the product last and never merges or reorders compositions. When a reactant composition equals the product composition (exactly the case being fixed), please verify that Reaction does not dedupe or cancel the common composition, which would shift the indices used in rxn.coeffs[: len(entry_list)] as well. The new test appears to cover the single-entry case only; add a case with a multi-entry list where one entry matches comp_dict.

@raashish1601

Copy link
Copy Markdown
Author

Checked the Reaction point: it does not merge or cancel a reactant that has the same composition as the product. For [Cu3P], [Cu3P, Cu] and [Cu, Cu3P] (plus the H/O dummies) the compositions stay in input order with the product last, e.g. coefficients [-1, 0, 0, 1] for Cu3P + H + O -> Cu3P, so coeffs[: len(entry_list)] and coeffs[-1] line up.

With a second entry next to the matching one, that entry gets a coefficient of 0, so process_multientry returns None as for any other zero coefficient. I added that case to the test in ca23d91 (both orders). tests/analysis/test_pourbaix_diagram.py passes (22 tests). CI on the branch is waiting for workflow approval.

@shyuep

shyuep commented Oct 8, 2026

Copy link
Copy Markdown
Member

Automated PR review generated by Claude (posted from @shyuep's account). Based on code and CI status only, nothing was run. Please sanity-check before acting.

Good: Correct, minimal fix for #4709. rxn.get_coeff(prod_comp) is ambiguous whenever a reactant has the same composition as the product, and rxn.coeffs[-1] removes the ambiguity. This is consistent with react_coeffs = rxn.coeffs[:len(entry_list)] just above, which already relies on positional order. The regression test covers single-entry, two-entry (both orders) and a full PourbaixDiagram.

CI: no status checks have run on this PR (zero checks reported), so there is no automated signal here. Worth triggering the workflow (or approving the fork run) before merge.

Substantive: none. The one assumption to confirm is that Reaction.coeffs always places the product last. It does for Reaction(reactants, products) today, but a short comment pointing at the ordering guarantee would make this robust to refactors.

Nit: the two-entry assertions (... is None) depend on the zero-coefficient filter below. A brief comment on why None is expected is already there; fine as is.

@raashish1601

Copy link
Copy Markdown
Author

The comment above that line already says the product is the last composition. Reaction keeps the compositions in the order they are passed (reactants, then products), and the new test would fail if that changed: the single-entry case checks that the weight is 1, which only holds when coeffs[-1] is the product. The test and Lint workflows are still waiting for approval.

This branch has not been deployed

No deployments
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.

PourbaixDiagram: process_multientry drops the single entry equal to comp_dict (Reaction.get_coeff returns −1 for an identical reactant/product)

2 participants