-
Notifications
You must be signed in to change notification settings - Fork 28
Adding changes from develop #436
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
Conversation
Sync develop branch with latest commits on master
Bumps [astral-sh/setup-uv](https://github.com/astral-sh/setup-uv) from 5 to 6. - [Release notes](https://github.com/astral-sh/setup-uv/releases) - [Commits](astral-sh/setup-uv@v5...v6) --- updated-dependencies: - dependency-name: astral-sh/setup-uv dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [pydash](https://github.com/dgilland/pydash) from 8.0.4 to 8.0.5. - [Changelog](https://github.com/dgilland/pydash/blob/develop/CHANGELOG.rst) - [Commits](dgilland/pydash@v8.0.4...v8.0.5) --- updated-dependencies: - dependency-name: pydash dependency-version: 8.0.5 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [importlib-metadata](https://github.com/python/importlib_metadata) from 8.5.0 to 8.7.0. - [Release notes](https://github.com/python/importlib_metadata/releases) - [Changelog](https://github.com/python/importlib_metadata/blob/main/NEWS.rst) - [Commits](python/importlib_metadata@v8.5.0...v8.7.0) --- updated-dependencies: - dependency-name: importlib-metadata dependency-version: 8.7.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [pymatgen](https://github.com/materialsproject/pymatgen) from 2025.3.10 to 2025.4.24. - [Release notes](https://github.com/materialsproject/pymatgen/releases) - [Changelog](https://github.com/materialsproject/pymatgen/blob/master/docs/CHANGES.md) - [Commits](materialsproject/pymatgen@v2025.3.10...v2025.4.24) --- updated-dependencies: - dependency-name: pymatgen dependency-version: 2025.4.24 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps mp-api from 0.45.3 to 0.45.5. --- updated-dependencies: - dependency-name: mp-api dependency-version: 0.45.5 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Deleted a .png file with a picture of snoopy the dog
Bumps [setuptools](https://github.com/pypa/setuptools) from 75.6.0 to 78.1.1. - [Release notes](https://github.com/pypa/setuptools/releases) - [Changelog](https://github.com/pypa/setuptools/blob/main/NEWS.rst) - [Commits](pypa/setuptools@v75.6.0...v78.1.1) --- updated-dependencies: - dependency-name: setuptools dependency-version: 78.1.1 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
…e oxidation states lists
Bumps [tornado](https://github.com/tornadoweb/tornado) from 6.4.2 to 6.5.1. - [Changelog](https://github.com/tornadoweb/tornado/blob/master/docs/releases.rst) - [Commits](tornadoweb/tornado@v6.4.2...v6.5.1) --- updated-dependencies: - dependency-name: tornado dependency-version: 6.5.1 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
… pre-filtering step
Bumps [tqdm](https://github.com/tqdm/tqdm) from 4.66.4 to 4.67.1. - [Release notes](https://github.com/tqdm/tqdm/releases) - [Commits](tqdm/tqdm@v4.66.4...v4.67.1) --- updated-dependencies: - dependency-name: tqdm dependency-version: 4.67.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [threadpoolctl](https://github.com/joblib/threadpoolctl) from 3.5.0 to 3.6.0. - [Release notes](https://github.com/joblib/threadpoolctl/releases) - [Changelog](https://github.com/joblib/threadpoolctl/blob/master/CHANGES.md) - [Commits](joblib/threadpoolctl@3.5.0...3.6.0) --- updated-dependencies: - dependency-name: threadpoolctl dependency-version: 3.6.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [requests](https://github.com/psf/requests) from 2.32.2 to 2.32.4. - [Release notes](https://github.com/psf/requests/releases) - [Changelog](https://github.com/psf/requests/blob/main/HISTORY.md) - [Commits](psf/requests@v2.32.2...v2.32.4) --- updated-dependencies: - dependency-name: requests dependency-version: 2.32.4 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [urllib3](https://github.com/urllib3/urllib3) from 2.2.2 to 2.5.0. - [Release notes](https://github.com/urllib3/urllib3/releases) - [Changelog](https://github.com/urllib3/urllib3/blob/main/CHANGES.rst) - [Commits](urllib3/urllib3@2.2.2...2.5.0) --- updated-dependencies: - dependency-name: urllib3 dependency-version: 2.5.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Generating compositions using custom oxidation states lists / Memory optimization
Oxidation states filters added to the smact_validity function
WalkthroughThis update introduces support for custom oxidation states in composition generation and filtering, enhances the flexibility of oxidation state selection in validity checks, and updates dependency versions. It also updates CI workflows to use the latest setup action, removes trailing blank lines in documentation, and includes minor code clarity improvements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant generate_composition_with_smact_custom
participant lookup_element_oxidation_states_custom
participant smact_filter
User->>generate_composition_with_smact_custom: Call with custom oxidation_states_set
generate_composition_with_smact_custom->>lookup_element_oxidation_states_custom: Load custom oxidation states
generate_composition_with_smact_custom->>smact_filter: Filter generated compositions using custom oxidation states
smact_filter-->>generate_composition_with_smact_custom: Return filtered compositions
generate_composition_with_smact_custom-->>User: Return DataFrame/results
sequenceDiagram
participant User
participant smact_validity
participant ICSD24OxStatesFilter
User->>smact_validity: Call with parameters (incl. consensus, commonality)
smact_validity->>ICSD24OxStatesFilter: Filter oxidation states as per parameters
ICSD24OxStatesFilter-->>smact_validity: Return filtered oxidation states
smact_validity-->>User: Return validity (True/False)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (11)
✨ 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #436 +/- ##
==========================================
+ Coverage 80.03% 80.66% +0.63%
==========================================
Files 33 33
Lines 2805 2871 +66
==========================================
+ Hits 2245 2316 +71
+ Misses 560 555 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
smact/screening.py (1)
510-511
: Remove redundant condition check.The condition
oxidation_states_set == "icsd24" or oxidation_states_set is None
will never haveoxidation_states_set is None
as true because it's already handled in the if block starting at line 491.Apply this diff to fix the redundant condition:
- elif oxidation_states_set == "icsd24" or oxidation_states_set is None: + elif oxidation_states_set == "icsd24": ox_combos = [el.oxidation_states_icsd24 for el in smact_elems]
🧹 Nitpick comments (3)
.github/workflows/ci.yml (1)
26-29
: Matrix step: watch for Windows + Python 3.13 re-enableNo functional issue with the v6 bump, but the comment above still mentions a “temporary” exclusion for Windows/3.13. When
setup-uv
fully supports 3.13 on Windows, remember to drop the exclusion to restore coverage parity.requirements.txt (1)
382-386
: pymatgen 2025.4.24: forward-compat with NumPy 2.0The pinned build is NumPy-2-ready but still flags
numpy.bool8
deprecations. Ensure the upper boundnumpy<3
inpyproject.toml
is retained until full migration.smact/screening.py (1)
491-503
: Consider extracting oxidation state filtering logic.The logic for filtering oxidation states when
oxidation_states_set
is None adds significant complexity to this function. Consider extracting it into a helper function for better readability and testability.def _get_filtered_oxidation_states(smact_elems, consensus, include_zero, commonality): """Get filtered oxidation states for elements using ICSD24OxStatesFilter.""" ox_filter = ICSD24OxStatesFilter() filtered_df = ox_filter.filter(consensus=consensus, include_zero=include_zero, commonality=commonality) oxidation_dict = { row["element"]: [int(x) for x in row["oxidation_state"].split()] for _, row in filtered_df.iterrows() } ox_combos = [] for el in smact_elems: ox_el = oxidation_dict.get(el.symbol, None) if ox_el is not None: ox_combos.append(ox_el) else: return None return ox_combosThen use it as:
if oxidation_states_set is None: ox_combos = _get_filtered_oxidation_states(smact_elems, consensus, include_zero, commonality) if ox_combos is None: return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ci.yml
(1 hunks).github/workflows/publish-to-pypi.yml
(1 hunks).github/workflows/update-changelog.yml
(1 hunks)README.md
(0 hunks)pyproject.toml
(1 hunks)requirements.txt
(4 hunks)smact/data_loader.py
(3 hunks)smact/screening.py
(5 hunks)smact/utils/crystal_space/generate_composition_with_smact.py
(3 hunks)smact/utils/oxidation.py
(3 hunks)
💤 Files with no reviewable changes (1)
- README.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
smact/screening.py (2)
smact/__init__.py (2)
neutral_ratios
(508-547)_gcd_recursive
(450-455)smact/utils/oxidation.py (2)
ICSD24OxStatesFilter
(13-210)filter
(26-81)
⏰ 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). (11)
- GitHub Check: test (3.12, windows-latest)
- GitHub Check: test (3.13, ubuntu-latest)
- GitHub Check: test (3.13, macos-latest)
- GitHub Check: test (3.11, ubuntu-latest)
- GitHub Check: test (3.10, windows-latest)
- GitHub Check: test (3.12, macos-latest)
- GitHub Check: test (3.12, ubuntu-latest)
- GitHub Check: test (3.11, macos-latest)
- GitHub Check: test (3.10, macos-latest)
- GitHub Check: test (3.11, windows-latest)
- GitHub Check: test (3.10, ubuntu-latest)
🔇 Additional comments (9)
.github/workflows/update-changelog.yml (1)
38-41
: Confirm action-v6 compatibility and cache behaviour
setup-uv@v6
introduces a new caching strategy and a stricter Python discovery. Double-check that (a) the existinguv run
invocations still pick up the correct interpreter and (b) the cache key is invalidated on lock-file changes, otherwise stale wheels may leak into the release build..github/workflows/publish-to-pypi.yml (1)
18-21
: Version bump looks safe but verify release artefact namesThe v6 action now defaults to a
dist/*
pattern that may differ whenuv build
produces tagged wheels (e.g.*.whl
,*.tar.gz
). Ensureactions/upload-artifact
still finds all artefacts after the bump.requirements.txt (3)
126-132
: importlib-metadata 8.7 → 8.7.0: check deprecated aliasesMinor, but 8.7 removes a few legacy entry-point helpers. If any runtime code imports
importlib_metadata.EntryPoint
, tests might miss it.
226-230
: mp-api 0.45.5 aligns with upstream, goodUpgrade matches the API-breaking release notes (new
robocrys
schema). Implementation insmact.utils.oxidation
is untouched, so LGTM.
372-376
: pydash 8.0.4 → 8.0.5: watch for tightened typing8.0.5 hardened a handful of type hints; pyright may now raise when
Any
is passed.smact/data_loader.py (2)
45-50
: Good performance optimization!The addition of checking for digits before yielding lines is a sensible optimization that will skip element entries without oxidation states, reducing unnecessary processing.
290-294
: Safe:_el_ox_states_custom
is always initialised before the “all” branch
- The initialisation logic at line 278 in
smact/data_loader.py
assigns_el_ox_states_custom
whenever anoxidation_states_set
is provided or if it was previouslyNone
.- The only invocation of
lookup_element_oxidation_states_custom("all", ...)
insmact/utils/crystal_space/generate_composition_with_smact.py
passes a non-nulloxidation_states_set
, ensuring_el_ox_states_custom
cannot beNone
.
No further changes are required.smact/utils/crystal_space/generate_composition_with_smact.py (1)
215-215
: Clarify the Francium electronegativity threshold.The code filters elements based on having Pauling electronegativity greater than or equal to Francium's. This seems like an arbitrary threshold that should be documented or made configurable.
Why is Francium's electronegativity (0.7) used as the threshold? This excludes only Francium itself. Consider:
- Making this threshold a parameter
- Adding a comment explaining the rationale
- Using a more explicit filter if the intention is just to exclude Francium
smact/screening.py (1)
395-395
: Good optimization using generator expression.Using a generator expression instead of a list comprehension is more memory-efficient when dealing with potentially large sets of oxidation states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
smact/tests/test_utils.py
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
smact/tests/test_utils.py (1)
smact/utils/crystal_space/generate_composition_with_smact.py (1)
generate_composition_with_smact
(42-145)
⏰ 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). (9)
- GitHub Check: test (3.13, ubuntu-latest)
- GitHub Check: test (3.12, ubuntu-latest)
- GitHub Check: test (3.11, ubuntu-latest)
- GitHub Check: test (3.10, windows-latest)
- GitHub Check: test (3.12, windows-latest)
- GitHub Check: test (3.12, macos-latest)
- GitHub Check: test (3.11, macos-latest)
- GitHub Check: test (3.10, ubuntu-latest)
- GitHub Check: test (3.11, windows-latest)
Description
Changes to the
smact.utils.oxidation
module allowing for the creation of custom oxidation states listsType of change
Please delete options that are not relevant.
How Has This Been Tested?
-pytest
Test Configuration:
Reviewers
@mention individuals who you specifically want to involve in the discussion for this pull request and mention why they are needed in the discussion/why they are needed to review the pull request.
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Dependency Updates
Chores