Revert "Revert "WIP: Support validity for mixed valence compounds"" #568
Conversation
WalkthroughThis PR introduces mixed-valence element handling to the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #568 +/- ##
===========================================
+ 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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @smact/screening.py:
- Around line 567-581: In _expand_mixed_valence_comp fix the typo 'electrnoeg'
to 'electroneg' in the zip() loop and all uses inside the function: rename the
loop variable from electrnoeg → electroneg and update
new_electronegs.extend([...]) and new_electronegs.append(...) calls to use
electroneg so the electronegs list is referenced correctly.
🧹 Nitpick comments (3)
smact/screening.py (3)
24-46: LGTM - Consider usingfrozensetfor immutability and O(1) lookups.The list of mixed-valence elements is chemically reasonable. A
frozensetwould provide immutability and faster membership checks, though the performance difference is negligible for this size.
567-568: Consider adding type hints to the helper function.Other functions in this module include type hints. Adding them here would improve consistency and maintainability.
🔎 Suggested type hints
def _expand_mixed_valence_comp( ox_combos: list[list[int]], stoichs: list[tuple[int, ...]], electronegs: list[float], elem_symbols: tuple[str, ...], ) -> tuple[list[list[int]], list[tuple[int, ...]], list[float]]: """Utility function to expand mixed valence elements in the composition."""
584-600: LGTM - Logic is correct.The function correctly iterates through oxidation state combinations and short-circuits on the first valid solution. The
TypeErrorhandling is consistent with the existing approach in the codebase.Consider adding type hints for consistency with other functions in this module.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
smact/screening.pysmact/tests/test_core.py
🧰 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)
⏰ 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). (7)
- GitHub Check: test (3.12, windows-latest)
- GitHub Check: test (3.12, ubuntu-latest)
- GitHub Check: test (3.11, windows-latest)
- GitHub Check: test (3.11, ubuntu-latest)
- GitHub Check: test (3.10, windows-latest)
- GitHub Check: test (3.13, ubuntu-latest)
- GitHub Check: test (3.10, ubuntu-latest)
🔇 Additional comments (3)
smact/screening.py (2)
458-478: LGTM!The new
mixed_valenceparameter is well-documented and defaults toFalsefor backward compatibility.
555-564: Verify that the unchangedthresholdis intentional after expansion.After
_expand_mixed_valence_comp, thestoichsare expanded butthresholdremains the original value from the unexpanded composition. For Fe₃O₄, the original threshold is 4, but after expansion, each Fe site has stoichiometry(1,). Theneutral_ratiosfunction usesthresholdto limit ratio search space.If the intent is to find ratios matching the expanded stoichiometries exactly, this should work. However, if the threshold should reflect the expanded composition's maximum stoichiometry (which would be 4 for O), please verify this is the intended behaviour.
smact/tests/test_core.py (1)
487-496: LGTM - Good basic coverage for the new feature.The test correctly validates Fe₃O₄ as the canonical mixed-valence compound. Consider adding additional test cases in future for broader coverage:
- A compound with no mixed-valence elements (should behave identically regardless of the flag)
- A compound that is valid without mixed-valence handling (to ensure the flag doesn't break existing behaviour)
- Other mixed-valence compounds like Mn₃O₄ or Pb₃O₄
| 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.
Fix typo: electrnoeg → electroneg.
The variable name electrnoeg on line 572 appears to be a typo of electroneg.
🔎 Proposed fix
- for el, ox, count, electrnoeg in zip(elem_symbols, ox_combos, stoichs, electronegs, strict=False):
+ for el, ox, count, electroneg 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([electroneg] * count[0])
new_stoichs.extend([(1,)] * count[0])
else:
new_ox_combos.append(ox)
- new_electronegs.append(electrnoeg)
+ new_electronegs.append(electroneg)
new_stoichs.append(count)📝 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.
| 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): | |
| """Utility function to expand mixed valence elements in the composition.""" | |
| new_ox_combos = [] | |
| new_stoichs = [] | |
| new_electronegs = [] | |
| for el, ox, count, electroneg 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([electroneg] * count[0]) | |
| new_stoichs.extend([(1,)] * count[0]) | |
| else: | |
| new_ox_combos.append(ox) | |
| new_electronegs.append(electroneg) | |
| new_stoichs.append(count) | |
| return new_ox_combos, new_stoichs, new_electronegs |
🤖 Prompt for AI Agents
In @smact/screening.py around lines 567-581, In _expand_mixed_valence_comp fix
the typo 'electrnoeg' to 'electroneg' in the zip() loop and all uses inside the
function: rename the loop variable from electrnoeg → electroneg and update
new_electronegs.extend([...]) and new_electronegs.append(...) calls to use
electroneg so the electronegs list is referenced correctly.
Reverts #460
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.