WIP: Support validity for mixed valence compounds #441
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adding changes from develop
Add Deepwiki badge
Update version number
WalkthroughA new mechanism for handling mixed valence elements was introduced in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant smact_validity
participant _is_valid_oxi_state
participant _expand_mixed_valence_comp
User->>smact_validity: Call with composition, mixed_valence flag
smact_validity->>_is_valid_oxi_state: Validate original composition
_is_valid_oxi_state-->>smact_validity: Return validity result
alt No valid combination and mixed_valence enabled
smact_validity->>_expand_mixed_valence_comp: Expand composition
_expand_mixed_valence_comp-->>smact_validity: Return expanded composition
smact_validity->>_is_valid_oxi_state: Validate expanded composition
_is_valid_oxi_state-->>smact_validity: Return validity result
end
smact_validity-->>User: Return True/False
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #441 +/- ##
===========================================
+ Coverage 80.66% 80.83% +0.16%
===========================================
Files 33 33
Lines 2871 2896 +25
===========================================
+ Hits 2316 2341 +25
Misses 555 555 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
added 2 commits
July 31, 2025 15:32
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
smact/screening.py (1)
584-601: Add comprehensive docstring for clarity.The function encapsulates important logic but lacks documentation.
def _is_valid_oxi_state(ox_combos, stoichs, threshold, electronegs, use_pauling_test=True): - """Utility function to check if there is a valid oxidation state solution.""" + """ + Check if there is a valid oxidation state combination that satisfies charge neutrality and electronegativity rules. + + Args: + ox_combos: List of possible oxidation states for each element + stoichs: List of stoichiometries for each element + threshold: Maximum stoichiometry value for neutral ratio calculations + electronegs: List of electronegativities for each element + use_pauling_test: Whether to apply Pauling electronegativity test (default: True) + + Returns: + bool: True if a valid oxidation state combination exists, False otherwise + """ for ox_states in itertools.product(*ox_combos):
🧹 Nitpick comments (2)
smact/screening.py (1)
24-45: Consider using a set for better lookup performance.Since
MIXED_VALENCE_ELEMENTSis only used for membership testing (line 559), converting it to a set would improve lookup performance from O(n) to O(1).-MIXED_VALENCE_ELEMENTS = [ +MIXED_VALENCE_ELEMENTS = { "Fe", "Mn", "Co", "Cu", "Ni", "V", "Ti", "Cr", "Nb", "Mo", "W", "Re", "Ru", "Os", "Pd", "Ag", "Au", "Sn", "Sb", "Bi", -] +}smact/tests/test_core.py (1)
487-496: Consider adding more comprehensive test cases.While the Fe₃O₄ test is excellent, consider adding tests for:
- Other mixed valence compounds (e.g., with Mn, V, or Cu)
- The edge case mentioned in PR objectives (Cs₂SbCl₆)
- Regular compounds with
mixed_valence=Trueto ensure no false positivesWould you like me to generate additional test cases to improve coverage of the mixed valence functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
smact/screening.py(4 hunks)smact/tests/test_core.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
smact/tests/test_core.py (1)
smact/screening.py (1)
smact_validity(448-564)
smact/screening.py (1)
smact/__init__.py (1)
_gcd_recursive(450-455)
🪛 Ruff (0.12.2)
smact/screening.py
482-482: import should be at the top-level of a file
(PLC0415)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (3.13, macos-latest)
- GitHub Check: test (3.11, windows-latest)
🔇 Additional comments (2)
smact/screening.py (2)
458-458: Good backward-compatible implementation.The addition of the
mixed_valenceparameter with a default value ofFalsemaintains backward compatibility while adding the new functionality.
555-564: Well-structured implementation of mixed valence logic.The refactoring cleanly separates the oxidation state validation logic and implements the mixed valence expansion algorithm as described in the PR objectives. The short-circuiting behaviour is preserved.
Comment on lines
+567
to
+582
| def _expand_mixed_valence_comp(ox_combos, stoichs, electronegs, elem_symbols): | ||
| """Utility function to expand mixed valence elements in the composition.""" | ||
| new_ox_combos = [] | ||
| new_stoichs = [] | ||
| new_electronegs = [] | ||
| for el, ox, count, electrnoeg in zip(elem_symbols, ox_combos, stoichs, electronegs, strict=False): | ||
| if el in MIXED_VALENCE_ELEMENTS: | ||
| new_ox_combos.extend([ox] * count[0]) | ||
| new_electronegs.extend([electrnoeg] * count[0]) | ||
| new_stoichs.extend([(1,)] * count[0]) | ||
| else: | ||
| new_ox_combos.append(ox) | ||
| new_electronegs.append(electrnoeg) | ||
| new_stoichs.append(count) | ||
| return new_ox_combos, new_stoichs, new_electronegs | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add docstring and fix typo in parameter name.
The function logic is correct, but needs documentation and has a typo in the parameter name.
def _expand_mixed_valence_comp(ox_combos, stoichs, electronegs, elem_symbols):
- """Utility function to expand mixed valence elements in the composition."""
+ """
+ Expand mixed valence elements in a composition by treating each as a separate species.
+
+ Args:
+ ox_combos: List of possible oxidation states for each element
+ stoichs: List of stoichiometries (as tuples) for each element
+ electronegs: List of electronegativities for each element
+ elem_symbols: List of element symbols
+
+ Returns:
+ tuple: Expanded (ox_combos, stoichs, electronegs) where mixed valence
+ elements are replicated according to their stoichiometry
+ """
new_ox_combos = []
new_stoichs = []
new_electronegs = []
- for el, ox, count, electrnoeg in zip(elem_symbols, ox_combos, stoichs, electronegs, strict=False):
+ for el, ox, count, electronegs in zip(elem_symbols, ox_combos, stoichs, electronegs, strict=False):
if el in MIXED_VALENCE_ELEMENTS:
new_ox_combos.extend([ox] * count[0])
- new_electronegs.extend([electrnoeg] * count[0])
+ new_electronegs.extend([electronegs] * count[0])
new_stoichs.extend([(1,)] * count[0])
else:
new_ox_combos.append(ox)
- new_electronegs.append(electrnoeg)
+ new_electronegs.append(electronegs)
new_stoichs.append(count)
return new_ox_combos, new_stoichs, new_electronegsAlso consider using strict=True in the zip() call to catch any length mismatches between the input lists, as this would indicate a programming error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _expand_mixed_valence_comp(ox_combos, stoichs, electronegs, elem_symbols): | |
| """Utility function to expand mixed valence elements in the composition.""" | |
| new_ox_combos = [] | |
| new_stoichs = [] | |
| new_electronegs = [] | |
| for el, ox, count, electrnoeg in zip(elem_symbols, ox_combos, stoichs, electronegs, strict=False): | |
| if el in MIXED_VALENCE_ELEMENTS: | |
| new_ox_combos.extend([ox] * count[0]) | |
| new_electronegs.extend([electrnoeg] * count[0]) | |
| new_stoichs.extend([(1,)] * count[0]) | |
| else: | |
| new_ox_combos.append(ox) | |
| new_electronegs.append(electrnoeg) | |
| new_stoichs.append(count) | |
| return new_ox_combos, new_stoichs, new_electronegs | |
| def _expand_mixed_valence_comp(ox_combos, stoichs, electronegs, elem_symbols): | |
| """ | |
| Expand mixed valence elements in a composition by treating each as a separate species. | |
| Args: | |
| ox_combos: List of possible oxidation states for each element | |
| stoichs: List of stoichiometries (as tuples) for each element | |
| electronegs: List of electronegativities for each element | |
| elem_symbols: List of element symbols | |
| Returns: | |
| tuple: Expanded (ox_combos, stoichs, electronegs) where mixed valence | |
| elements are replicated according to their stoichiometry | |
| """ | |
| new_ox_combos = [] | |
| new_stoichs = [] | |
| new_electronegs = [] | |
| for el, ox, count, electronegs in zip(elem_symbols, ox_combos, stoichs, electronegs, strict=False): | |
| if el in MIXED_VALENCE_ELEMENTS: | |
| new_ox_combos.extend([ox] * count[0]) | |
| new_electronegs.extend([electronegs] * count[0]) | |
| new_stoichs.extend([(1,)] * count[0]) | |
| else: | |
| new_ox_combos.append(ox) | |
| new_electronegs.append(electronegs) | |
| new_stoichs.append(count) | |
| return new_ox_combos, new_stoichs, new_electronegs |
🤖 Prompt for AI Agents
In smact/screening.py around lines 567 to 582, add a clear docstring explaining
the purpose and parameters of the _expand_mixed_valence_comp function. Fix the
typo in the parameter name from 'electrnoeg' to 'electronegs' in the zip() call
and throughout the function. Also, change the zip() call to use strict=True to
ensure input lists have matching lengths and catch potential programming errors
early.
AntObi
assigned
aronwalsh,
keeeto,
dandavies99,
ryannduma,
hspark1212,
Panyalak and
KingaMas and unassigned
AntObi
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR is WIP while we finalise the list of supported mixed valence elements.
Added support for mixed valence compounds in the
smact_validityfunction. The default behaviour of the function has not changed. For example:The algorithm is as follows:
The algorithm will fail if the reduced formula does not support mixed valence elements due to limited multiplicity. For example, Cs2SbCl6, which contains Sb3+ and Sb5+. To overcome this, a future fix could be to add another option to repeat the composition up to a fixed number (i.e., Cs4SbSbCl12).
This PR addresses #196 and #418.
Type of change
How Has This Been Tested?
Added a
test_smact_validity_mixed_valenceintests_core.py.Reviewers
@Panyalak @dandavies99
Checklist
Summary by CodeRabbit