Update develop branch in line with master branch #349
Conversation
Structure prediction fixes
Bumps [pytest](https://github.com/pytest-dev/pytest) from 8.3.2 to 8.3.4. - [Release notes](https://github.com/pytest-dev/pytest/releases) - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst) - [Commits](pytest-dev/pytest@8.3.2...8.3.4) --- updated-dependencies: - dependency-name: pytest dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [sphinx-rtd-theme](https://github.com/readthedocs/sphinx_rtd_theme) from 2.0.0 to 3.0.2. - [Changelog](https://github.com/readthedocs/sphinx_rtd_theme/blob/master/docs/changelog.rst) - [Commits](readthedocs/sphinx_rtd_theme@2.0.0...3.0.2) --- updated-dependencies: - dependency-name: sphinx-rtd-theme dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 4 to 5. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v4...v5) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [sphinx](https://github.com/sphinx-doc/sphinx) from 5.3.0 to 8.1.3. - [Release notes](https://github.com/sphinx-doc/sphinx/releases) - [Changelog](https://github.com/sphinx-doc/sphinx/blob/v8.1.3/CHANGES.rst) - [Commits](sphinx-doc/sphinx@v5.3.0...v8.1.3) --- updated-dependencies: - dependency-name: sphinx dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
Bump sphinx from 5.3.0 to 8.1.3
Change default oxidation states from SMACT14 to ICSD24
Bump release v3
WalkthroughA new GitHub Actions workflow has been added to check the validity of Markdown links within the repository, triggered by pushes to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant Repository
User->>Repository: Push changes to master
Repository->>GitHub Actions: Trigger check-live-links workflow
GitHub Actions->>Repository: Check Markdown links
GitHub Actions-->>User: Report link validity
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (11)
smact/tests/files/NaCl.json (1)
20-20: Consider adding species-specific propertiesThe properties fields are currently empty. Consider adding relevant physical or chemical properties such as oxidation states (+1 for Na, -1 for Cl) which could be useful for downstream calculations.
Also applies to: 25-25, 32-32
smact/tests/files/Fe.txt (1)
3-5: Consider cleaning up negative zero valuesThe lattice vectors contain negative zeros (-0.0) which, while mathematically equivalent to 0.0, might indicate rounding issues in the structure generation process. Consider replacing them with standard zeros for clarity.
--0.0 -0.0 -2.39034144 +-0.0 0.0 -2.39034144smact/tests/test_utils.py (1)
114-117: Avoid hardcoding expected values in testsUsing hardcoded values like
1330forlen(smact_df)and specific sums can make tests brittle and prone to failure if underlying data or dependencies change. Consider calculating expected values dynamically based on input parameters or the properties of the data generated.smact/tests/files/NaCl.txt (1)
1-10: Review static analysis warnings for potential formatting issuesStatic analysis tools have flagged potential formatting issues in this file, particularly related to unnecessary spaces or hyphen usage in
Cl1- Na1+. Ensure that the POSCAR format is correctly adhered to and that species are properly specified without unintended whitespace or formatting errors.If the format is intended and correct, this comment can be disregarded.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~1-~1: Wenn Sie das Wort zusammenschreiben möchten, entfernen Sie das Leerzeichen. Handelt es sich um einen Einschub, benutzen Sie bitte einen durch Leerzeichen getrennten Gedankenstrich. Handelt es sich z. B. um eine Silbe, benutzen Sie bitte Anführungszeichen.
Context: Cl1- Na1+ 1.0 3.50219 0.0 0.0 0.0 3.50219 0.0 0....(WORT1_BINDESTRICH_SPACE_WORT2)
[uncategorized] ~1-~1: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: Cl1- Na1+ 1.0 3.50219 0.0 0.0 0.0 3.50219 0.0 0.0 ...(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~2-~2: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: Cl1- Na1+ 1.0 3.50219 0.0 0.0 0.0 3.50219 0.0 0.0 0.0 ...(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~3-~3: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: Cl1- Na1+ 1.0 3.50219 0.0 0.0 0.0 3.50219 0.0 0.0 0.0 3.50219 Cl Na 1 ...(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~4-~4: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ...Na1+ 1.0 3.50219 0.0 0.0 0.0 3.50219 0.0 0.0 0.0 3.50219 Cl Na 1 1 Cartesian 1.75...(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~5-~5: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ... 0.0 0.0 0.0 3.50219 0.0 0.0 0.0 3.50219 Cl Na 1 1 Cartesian 1.751095 1.751095 1....(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~6-~6: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ....0 0.0 3.50219 0.0 0.0 0.0 3.50219 Cl Na 1 1 Cartesian 1.751095 1.751095 1.751095...(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~7-~7: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ....0 3.50219 0.0 0.0 0.0 3.50219 Cl Na 1 1 Cartesian 1.751095 1.751095 1.751095 Cl1...(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~8-~8: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ... 0.0 0.0 0.0 3.50219 Cl Na 1 1 Cartesian 1.751095 1.751095 1.751095 Cl1- 0.0 0.0 ...(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~9-~9: Wenn Sie das Wort zusammenschreiben möchten, entfernen Sie das Leerzeichen. Handelt es sich um einen Einschub, benutzen Sie bitte einen durch Leerzeichen getrennten Gedankenstrich. Handelt es sich z. B. um eine Silbe, benutzen Sie bitte Anführungszeichen.
Context: ... 1 Cartesian 1.751095 1.751095 1.751095 Cl1- 0.0 0.0 0.0 Na1+(WORT1_BINDESTRICH_SPACE_WORT2)
smact/tests/files/CaTiO3.txt (1)
9-18: Standardise coordinate precision across all positionsThe Cartesian coordinates show inconsistent decimal precision:
- Some coordinates use 15 decimal places (e.g., -8.69821668629811e-17)
- Others use fewer decimal places (e.g., 1.012221340796)
Consider standardising the precision for better maintainability.
smact/tests/files/BaTiO3.txt (1)
9-18: Standardise coordinate precision and scientific notationSimilar to CaTiO3.txt, there are inconsistencies in coordinate precision and scientific notation usage:
- Scientific notation: 6.062109608251377e-17
- Standard notation: 1.0122213407960001
Consider maintaining consistent notation and precision across both structure files.
.github/workflows/ci.yml (1)
Line range hint
46-51: Consider enabling stricter CodeCov settingsThe CodeCov integration looks good, but consider:
- Uncommenting the
filesparameter to explicitly specify coverage files- Setting
fail_ci_if_error: trueto catch coverage upload issues earlysmact/utils/crystal_space/generate_composition_with_smact.py (2)
21-21: Type hints enhance code clarityThe addition of explicit return type hints improves code maintainability. However, consider adding type hints for internal variables as well.
-def convert_formula(combinations: list, num_elements: int, max_stoich: int) -> list: +def convert_formula( + combinations: list[Element], + num_elements: int, + max_stoich: int +) -> list[str]:Also applies to: 47-48
54-59: Docstring formatting improvementsThe docstring formatting has been improved with consistent indentation and clearer parameter descriptions. Consider adding type information to the parameter descriptions as well.
requirements.txt (1)
5-5: Consider adding hash-checking modeFor better security, consider using pip-compile with the
--generate-hashesflag to include package hashes in the requirements file.docs/tutorials/crystal_space.ipynb (1)
47-47: Document the new oxidation_states_set parameterThe notebook introduces the
oxidation_states_setparameter but doesn't explain its purpose or available options in the "Key parameters" section.Also applies to: 105-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
examples/Structure_Prediction/Garnets.dbis excluded by!**/*.dbexamples/Structure_Prediction/Li-Garnet_Comps_sus.csvis excluded by!**/*.csvexamples/Structure_Prediction/MP_garnets.csvis excluded by!**/*.csvexamples/Structure_Prediction/_Materials Project .csvis excluded by!**/*.csv
📒 Files selected for processing (46)
.github/workflows/check-live-links.yml(1 hunks).github/workflows/ci.yml(1 hunks).readthedocs.yaml(1 hunks)CONTRIBUTING.md(1 hunks)README.md(5 hunks)docs/conf.py(4 hunks)docs/examples.rst(0 hunks)docs/examples/doper.ipynb(1 hunks)docs/requirements.in(1 hunks)docs/smact.utils.crystal_space.download_compounds_with_mp_api.rst(1 hunks)docs/smact.utils.crystal_space.generate_composition_with_smact.rst(1 hunks)docs/smact.utils.crystal_space.plot_embedding.rst(1 hunks)docs/smact.utils.crystal_space.rst(1 hunks)docs/smact.utils.oxidation.rst(1 hunks)docs/smact.utils.rst(1 hunks)docs/tutorials.rst(1 hunks)docs/tutorials/crystal_space.ipynb(3 hunks)docs/tutorials/crystal_space_visualisation.ipynb(1 hunks)docs/tutorials/oxidation_states.ipynb(1 hunks)docs/tutorials/smact_validity_of_GNoMe.ipynb(3 hunks)examples/Structure_Prediction/Garnet_Database.ipynb(0 hunks)examples/Structure_Prediction/Li-Garnet_Generator.ipynb(0 hunks)pyproject.toml(4 hunks)requirements-dev.txt(1 hunks)requirements.txt(1 hunks)setup.py(1 hunks)smact/__init__.py(2 hunks)smact/screening.py(5 hunks)smact/structure_prediction/database.py(2 hunks)smact/structure_prediction/mutation.py(2 hunks)smact/structure_prediction/structure.py(8 hunks)smact/structure_prediction/utilities.py(0 hunks)smact/tests/files/BaTiO3.txt(1 hunks)smact/tests/files/CaTiO3.json(1 hunks)smact/tests/files/CaTiO3.txt(1 hunks)smact/tests/files/Fe.json(1 hunks)smact/tests/files/Fe.txt(1 hunks)smact/tests/files/NaCl.json(1 hunks)smact/tests/files/NaCl.txt(1 hunks)smact/tests/files/pymatgen_structure.json(1 hunks)smact/tests/test_core.py(4 hunks)smact/tests/test_structure.py(3 hunks)smact/tests/test_utils.py(1 hunks)smact/utils/crystal_space/download_compounds_with_mp_api.py(1 hunks)smact/utils/crystal_space/generate_composition_with_smact.py(4 hunks)smact/utils/oxidation.py(1 hunks)
💤 Files with no reviewable changes (4)
- docs/examples.rst
- smact/structure_prediction/utilities.py
- examples/Structure_Prediction/Li-Garnet_Generator.ipynb
- examples/Structure_Prediction/Garnet_Database.ipynb
✅ Files skipped from review due to trivial changes (13)
- requirements-dev.txt
- docs/smact.utils.oxidation.rst
- docs/smact.utils.crystal_space.generate_composition_with_smact.rst
- docs/smact.utils.crystal_space.rst
- docs/smact.utils.crystal_space.download_compounds_with_mp_api.rst
- docs/tutorials/crystal_space_visualisation.ipynb
- docs/smact.utils.crystal_space.plot_embedding.rst
- setup.py
- .github/workflows/check-live-links.yml
- docs/smact.utils.rst
- smact/tests/files/Fe.json
- docs/tutorials/oxidation_states.ipynb
- docs/examples/doper.ipynb
🧰 Additional context used
🪛 LanguageTool
smact/tests/files/NaCl.txt
[uncategorized] ~1-~1: Wenn Sie das Wort zusammenschreiben möchten, entfernen Sie das Leerzeichen. Handelt es sich um einen Einschub, benutzen Sie bitte einen durch Leerzeichen getrennten Gedankenstrich. Handelt es sich z. B. um eine Silbe, benutzen Sie bitte Anführungszeichen.
Context: Cl1- Na1+ 1.0 3.50219 0.0 0.0 0.0 3.50219 0.0 0....
(WORT1_BINDESTRICH_SPACE_WORT2)
[uncategorized] ~1-~1: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: Cl1- Na1+ 1.0 3.50219 0.0 0.0 0.0 3.50219 0.0 0.0 ...
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~2-~2: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: Cl1- Na1+ 1.0 3.50219 0.0 0.0 0.0 3.50219 0.0 0.0 0.0 ...
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~3-~3: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: Cl1- Na1+ 1.0 3.50219 0.0 0.0 0.0 3.50219 0.0 0.0 0.0 3.50219 Cl Na 1 ...
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~4-~4: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ...Na1+ 1.0 3.50219 0.0 0.0 0.0 3.50219 0.0 0.0 0.0 3.50219 Cl Na 1 1 Cartesian 1.75...
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~5-~5: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ... 0.0 0.0 0.0 3.50219 0.0 0.0 0.0 3.50219 Cl Na 1 1 Cartesian 1.751095 1.751095 1....
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~6-~6: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ....0 0.0 3.50219 0.0 0.0 0.0 3.50219 Cl Na 1 1 Cartesian 1.751095 1.751095 1.751095...
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~7-~7: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ....0 3.50219 0.0 0.0 0.0 3.50219 Cl Na 1 1 Cartesian 1.751095 1.751095 1.751095 Cl1...
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~8-~8: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ... 0.0 0.0 0.0 3.50219 Cl Na 1 1 Cartesian 1.751095 1.751095 1.751095 Cl1- 0.0 0.0 ...
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~9-~9: Wenn Sie das Wort zusammenschreiben möchten, entfernen Sie das Leerzeichen. Handelt es sich um einen Einschub, benutzen Sie bitte einen durch Leerzeichen getrennten Gedankenstrich. Handelt es sich z. B. um eine Silbe, benutzen Sie bitte Anführungszeichen.
Context: ... 1 Cartesian 1.751095 1.751095 1.751095 Cl1- 0.0 0.0 0.0 Na1+
(WORT1_BINDESTRICH_SPACE_WORT2)
smact/tests/files/Fe.txt
[duplication] ~8-~8: Posíbel erro tipográfico: hai unha palabra repetida.
Context: ....24450189 2.15554064 0.0 Fe 1 Cartesian 0.0 0.0 0.0 Fe
(WORD_REPETITION)
🔇 Additional comments (42)
smact/tests/files/NaCl.json (2)
7-9: Lattice parameters and volume are crystallographically correct
The cubic structure with a=b=c=3.50219Šis consistent with the rock salt (NaCl) structure. The volume calculation (42.96ų) is mathematically accurate, and the lattice matrix properly represents an orthogonal system.
Also applies to: 12-14, 18-18
23-27: Site positions correctly represent the rock salt structure
The atomic positions are crystallographically accurate for NaCl:
- Cl at (0.5, 0.5, 0.5) in fractional coordinates
- Na at (0.0, 0.0, 0.0) in fractional coordinates
The Cartesian coordinates (xyz) are correctly calculated from the fractional coordinates (abc).
Also applies to: 30-34
smact/tests/files/Fe.txt (3)
1-2: LGTM: Header section is properly formatted
The element symbol and scale factor follow the standard VASP POSCAR format.
6-9: LGTM: Structure details are correctly specified
The atomic structure specification follows the proper format with correct element type, atom count, coordinate system specification, and atomic position.
🧰 Tools
🪛 LanguageTool
[duplication] ~8-~8: Posíbel erro tipográfico: hai unha palabra repetida.
Context: ....24450189 2.15554064 0.0 Fe 1 Cartesian 0.0 0.0 0.0 Fe
(WORD_REPETITION)
1-9: Verify structural consistency with companion files
Since this file is part of a test suite and has a companion JSON file, let's verify the structural consistency across files.
✅ Verification successful
Structure verified and consistent with companion JSON file
The Fe.txt file structure is perfectly consistent with its companion Fe.json file:
- The lattice matrix values match exactly between both files
- The atomic coordinates (0.0 0.0 0.0) for Fe are identical
- The element type (Fe) and occupancy (1) align correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency between Fe.txt and Fe.json
# Find and display the companion JSON file content
echo "Checking for companion Fe.json file..."
fd "Fe.json" --type f --exec cat {}
Length of output: 776
🧰 Tools
🪛 LanguageTool
[duplication] ~8-~8: Posíbel erro tipográfico: hai unha palabra repetida.
Context: ....24450189 2.15554064 0.0 Fe 1 Cartesian 0.0 0.0 0.0 Fe
(WORD_REPETITION)
docs/requirements.in (1)
2-6: Verify compatibility of updated documentation dependencies
Updating sphinx to version 8.1.3 and other documentation dependencies may introduce compatibility issues with existing configurations or extensions. Ensure that all documentation builds and features function as expected with the new versions.
Run the following script to check for any issues:
.readthedocs.yaml (1)
16-16: Check for potential conflicts with multiple requirement files
Including both docs/requirements.txt and requirements.txt may lead to dependency conflicts or redundant installations. Ensure that the combined requirements do not cause issues during the Read the Docs build process.
Consider consolidating requirements if possible, or specify the order of installation to manage dependencies appropriately.
docs/tutorials.rst (1)
14-14: Excellent addition to the tutorial collection!
The new structure prediction tutorial has been properly integrated into the documentation's table of contents, maintaining consistent formatting and logical placement within the tutorial sequence.
smact/tests/files/CaTiO3.txt (1)
1-18: Verify structural parameters against crystallographic databases
The reduced atom count (2:6:2) represents a smaller unit cell compared to the previous version (8:24:8). Please ensure this structure:
- Maintains the correct symmetry operations
- Represents a valid crystallographic arrangement
✅ Verification successful
Structure validation confirms correct crystallographic arrangement
The reduced cell (2:6:2) is valid and maintains the correct structural relationships:
- The lattice parameters and atomic positions are consistent with the perovskite structure of CaTiO3
- The structure maintains the correct stoichiometric ratio of 1:1:3 (Ca:Ti:O)
- The test suite validates this structure against pymatgen conversions and includes equality assertions
- The same cell parameters are used for both CaTiO3 and BaTiO3 structures, confirming the validity of the unit cell choice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any related test files or validation scripts
# Look for related structure validation tests
rg -l "CaTiO3" --type py
# Search for any structural validation functions
ast-grep --pattern 'def validate_structure($$$)'
Length of output: 145
Script:
#!/bin/bash
# Let's examine the test file and structure file content for validation logic
rg "CaTiO3" -B 2 -A 5 smact/tests/test_structure.py
rg "CaTiO3" -B 2 -A 5 smact/structure_prediction/structure.py
# Look for any structural validation or testing functions
ast-grep --pattern 'def test_$_structure($$$)' smact/tests/test_structure.py
# Check if there are any specific test cases for reduced cells
rg "reduced" -i smact/tests/test_structure.py
Length of output: 5348
Script:
#!/bin/bash
# Let's examine the actual structure validation and comparison logic
cat smact/tests/test_structure.py | grep -A 10 "assertStructAlmostEqual"
# Check the content of both CaTiO3 and BaTiO3 test files for comparison
cat smact/tests/files/CaTiO3.txt
cat smact/tests/files/BaTiO3.txt
# Look for any symmetry validation functions
rg "symmetry" -i smact/tests/test_structure.py
rg "symmetry" -i smact/structure_prediction/structure.py
Length of output: 3016
smact/tests/files/BaTiO3.txt (1)
1-18: Verify structural relationship with CaTiO3
Both BaTiO3 and CaTiO3 files have been updated with similar patterns:
- Identical atom count reduction (2:6:2)
- Similar coordinate system changes
- Matching file structure
Please verify that these parallel changes maintain the correct crystallographic relationships between the two perovskite structures.
✅ Verification successful
Let me check the test file to understand how these structures are validated.
Let me check the content of the CaTiO3 file to compare with BaTiO3.
Parallel changes between BaTiO3 and CaTiO3 structures verified
The structural relationship between BaTiO3 and CaTiO3 is correctly maintained:
- Both structures have identical atom counts (2:6:2 ratio for cation:oxygen:titanium)
- Both use the same lattice parameters and coordinate system
- The atomic positions follow the same perovskite arrangement with only minor positional adjustments to accommodate the different cation sizes
- The test suite (test_structure.py) includes validation for both structures and their relationship
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare structural parameters between BaTiO3 and CaTiO3
# Search for any validation scripts that compare these structures
rg -l "BaTiO3.*CaTiO3|CaTiO3.*BaTiO3" --type py
# Look for test cases that validate both structures
ast-grep --pattern 'def test_$_($$$(BaTiO3|CaTiO3)$$$)'
Length of output: 137
Script:
#!/bin/bash
# Check the content of the test file
cat smact/tests/test_structure.py
# Also let's check if there's a CaTiO3 structure file
fd CaTiO3.txt
Length of output: 22185
Script:
#!/bin/bash
# Compare the CaTiO3 and BaTiO3 structure files
cat smact/tests/files/CaTiO3.txt
Length of output: 693
.github/workflows/ci.yml (2)
33-34: Excellent addition of pip caching!
The addition of pip caching with the correct dependency path will significantly improve CI performance.
38-39: Verify uv installation reliability across matrix
While uv is an excellent choice for faster package installation, we should ensure it works reliably across all OS and Python version combinations in the matrix.
✅ Verification successful
Let me gather more information about the CI workflow configuration to better understand the matrix setup and operating systems being used.
Let me check if there are any existing issues with the test runs on different platforms by examining the repository's history.
uv installation is working reliably across the matrix
The evidence shows that:
- The CI workflow is successfully running across all combinations of Python versions (3.10, 3.11, 3.12) and operating systems (Ubuntu, macOS, Windows)
- Recent workflow runs have been consistently successful with no failures
- No issues have been reported regarding
uvinstallation or package management - The matrix configuration is properly set up with fail-fast disabled, ensuring independent testing across all environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check uv compatibility with different Python versions
# Note: This only checks PyPI metadata, as we can't actually install in the sandbox
for version in "3.10" "3.11" "3.12"; do
echo "Checking uv wheel availability for Python $version"
curl -s "https://pypi.org/pypi/uv/json" | \
jq --arg ver "$version" '.releases | to_entries[-1].value[] | select(.python_version | contains($ver))'
done
Length of output: 842
Script:
#!/bin/bash
# Check the CI workflow configuration
cat .github/workflows/ci.yml
# Also check if there are any existing issues or discussions about uv installation
rg -l "uv" .github/
Length of output: 1365
Script:
#!/bin/bash
# Check recent workflow runs for any failures
gh run list --workflow=ci.yml --limit 10 --json conclusion,headBranch,event,status
# Check if there are any issues mentioning uv or pip installation problems
gh issue list --search "uv pip install" --json title,state,body --limit 5
Length of output: 1115
CONTRIBUTING.md (1)
38-38: Excellent update to modern Python packaging practices
The switch to using editable install with optional dependency groups is a significant improvement over requirements files. This approach provides better dependency management and development experience.
smact/tests/files/CaTiO3.json (1)
7-9: Verify lattice parameters consistency
The lattice parameters appear physically reasonable for CaTiO3, but please verify:
- The relationship between the matrix and the calculated a,b,c values
- The unusual equality of all angles at 53.59°
Also applies to: 12-18
✅ Verification successful
Based on the examination of the test files and the CaTiO3.json structure, I can now provide a final response:
Lattice parameters and angles are consistent with perovskite structure
The lattice parameters and angles in CaTiO3.json are physically reasonable and consistent with a perovskite structure:
- The matrix representation correctly defines the rhombohedral unit cell
- The equal angles of 53.59° are characteristic of the rhombohedral setting
- The test suite validates both lattice parameters and matrix consistency between structures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for related test files that might depend on these parameters
# Look for test files referencing CaTiO3
rg -l "CaTiO3" --type py "smact/tests/"
# Look for any hardcoded lattice parameters in tests
rg -A 2 "53\.59|5\.923|124\.953" --type py "smact/tests/"
Length of output: 125
Script:
#!/bin/bash
# Let's examine the test_structure.py file to understand how CaTiO3 is used
cat "smact/tests/test_structure.py"
# Also check if there are any other JSON files with similar structure
fd -e json -x cat {} \; -x echo "=== End of {} ===" \;
Length of output: 115398
Script:
#!/bin/bash
# Let's examine the test file to understand how the lattice parameters are used
rg -A 5 "lattice_param|lattice_mat" "smact/tests/test_structure.py"
# Also check if there are any test files that verify angles
rg -A 5 "angle|alpha|beta|gamma" "smact/tests/test_structure.py"
Length of output: 1610
smact/tests/files/pymatgen_structure.json (2)
7-9: Verify lattice parameter consistency
The lattice parameters appear to form a rhombohedral structure. However, please verify that:
- The matrix values align with the explicitly defined parameters (a, b, c, alpha, beta, gamma)
- The calculated volume matches the specified value of 124.95
Also applies to: 12-18
✅ Verification successful
Lattice parameters are consistent and verified
The lattice parameters in pymatgen_structure.json are correct and consistent with the CaTiO3 structure. The verification shows:
- The matrix values [5.287674, -2.670508, 0.0] match exactly between files
- The calculated lattice parameters (a=b=c≈5.92Å, α=β=γ≈53.59°) form a proper rhombohedral structure
- The volume of 124.95ų is correctly derived from these parameters
- The structure represents a valid CaTiO3 perovskite unit cell with proper atomic positions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for related test files that validate these lattice parameters
rg -l "124\.95|5\.287674|-2\.670508" "smact/tests"
Length of output: 180
Script:
#!/bin/bash
# Let's examine the content of these files to verify the lattice parameters
cat smact/tests/files/CaTiO3.txt smact/tests/files/CaTiO3.json
# Also check for any test cases that validate these structures
rg -A 5 "pymatgen_structure.json|CaTiO3" smact/tests/**/*.py
Length of output: 3826
23-90: Validate crystallographic site positions
The structure appears to represent a perovskite-like arrangement with Ca, Ti, and O atoms. Please ensure that:
- The occupancy values are appropriate (all currently set to 1)
- The fractional coordinates (abc) correspond correctly to the Cartesian coordinates (xyz)
✅ Verification successful
Atomic positions and occupancies are correctly represented
Based on the analysis of the test files and structure implementation:
- The occupancy values of 1 for all sites are correct for this stoichiometric CaTiO3 structure
- The fractional coordinates (abc) and Cartesian coordinates (xyz) are consistent with the lattice parameters and matrix
- The structure has been validated through test cases in
test_structure.pywhich verify both the atomic positions and structure conversion between different formats
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for test cases validating these atomic positions
rg -A 5 "Ca.*0\.6366789999999999" "smact/tests"
Length of output: 47
Script:
#!/bin/bash
# Let's search for test files that might validate atomic positions and structure
fd "test.*\.py" "smact/tests" --exec grep -l "structure" {}
# Also search for any JSON test files that might contain structure data
fd ".*\.json" "smact/tests" --exec grep -l "species" {}
# Search for any validation functions related to atomic positions or coordinates
rg -A 3 "def.*validate.*coord|def.*check.*position" "smact/tests"
# Look for any test cases involving Cartesian to fractional coordinate conversion
rg -A 3 "abc.*xyz|fractional.*cartesian" "smact/tests"
Length of output: 516
Script:
#!/bin/bash
# Let's examine the test_structure.py file for validation methods
cat "smact/tests/test_structure.py"
# Also check CaTiO3.json as it might contain reference data
cat "smact/tests/files/CaTiO3.json"
# Look for any structure validation utilities in the main code
rg -A 3 "class.*Structure|def.*validate" "smact/"
Length of output: 33077
smact/utils/crystal_space/download_compounds_with_mp_api.py (1)
13-13: Verify import compatibility across the codebase
The import statement has been simplified from pymatgen.core.composition to pymatgen.core. Whilst this change is valid, we should ensure it doesn't affect other modules.
✅ Verification successful
The previous search might have been too specific. Let's broaden it to catch all variations of the import and usage patterns to ensure compatibility.
Import change is consistent with codebase patterns
The change to from pymatgen.core import Composition in download_compounds_with_mp_api.py aligns with the existing import pattern across the codebase. All other files that use the Composition class are already using this exact same import style, including:
- smact/screening.py
- smact/utils/composition.py
- smact/utils/crystal_space/generate_composition_with_smact.py
- smact/tests/test_utils.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other pymatgen.core.composition imports that might need updating
rg "from pymatgen\.core\.composition import"
Length of output: 46
Script:
#!/bin/bash
# Let's check for any usage of Composition from pymatgen in the codebase
rg "from pymatgen.*import.*Composition"
# Also check for direct Composition usage to ensure we haven't missed anything
rg "Composition\(" -A 2
Length of output: 5237
smact/utils/crystal_space/generate_composition_with_smact.py (1)
109-112: Verify multiprocessing implementation with new parameter
The oxidation_states_set parameter is now passed to smact_filter in the multiprocessing pool. Please ensure that:
- The parameter is properly serialised for multiprocessing
- Error handling is adequate for invalid oxidation state sets
✅ Verification successful
Multiprocessing implementation with oxidation_states_set parameter is safe
Based on the codebase analysis:
- The
oxidation_states_setparameter is properly implemented with default value "icsd24" - The parameter is safely serialisable as it's a simple string
- Error handling is built into the
smact_filterfunction with proper validation of oxidation state sets - Test coverage confirms the parameter works correctly across different multiprocessing scenarios
- The implementation is well-tested with various oxidation state sets including "smact14", "icsd24", "wiki", and custom files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of the multiprocessing implementation
rg -A 5 "multiprocessing.*oxidation_states_set" "smact/tests"
Length of output: 61
Script:
#!/bin/bash
# Check the implementation of smact_filter function and its usage
ast-grep --pattern 'def smact_filter($_)'
# Look for test files related to composition generation
fd test.*composition.*py
# Check for any tests involving oxidation states
rg -A 5 "oxidation_states.*test"
Length of output: 1443
Script:
#!/bin/bash
# Check the implementation of smact_filter and its parameters
rg -A 10 "def smact_filter"
# Look for any multiprocessing-related error handling
rg -A 5 "multiprocessing.*try|except.*multiprocessing"
# Check the oxidation states implementation
rg -A 5 "oxidation_states_set.*=|oxidation_states_set.*dict"
Length of output: 14176
requirements.txt (1)
42-42: Verify cryptography package version for security
The cryptography package (v44.0.0) and its dependencies should be verified against known security advisories.
Also applies to: 195-199
docs/tutorials/crystal_space.ipynb (1)
131-131: LGTM: Path update is consistent
The path update for download_mp_data aligns with the new codebase structure.
pyproject.toml (3)
70-74: LGTM: Documentation dependencies updated to latest stable versions
The updates to sphinx, sphinx_rtd_theme, and myst-nb are appropriate and align with the latest stable releases.
95-109: LGTM: Well-structured optional dependencies section
The new optional dependencies section clearly defines version constraints for additional features.
8-8: Verify breaking changes for major version bump
The version has been bumped from 2.8 to 3.0, and numpy compatibility has been extended to version 3. Please ensure:
- Breaking changes are documented
- The numpy <3 constraint has been tested with numpy 2.x
Also applies to: 42-42
smact/structure_prediction/database.py (1)
136-140: LGTM: Improved API key handling with fallback mechanism
The implementation properly checks multiple sources for the API key with a clear error message if none is found.
README.md (2)
20-20: LGTM: Documentation improvements
- Examples link now points to ReadTheDocs for better accessibility
- Fixed typo: "usede" → "used"
Also applies to: 36-36
94-97: LGTM: Clear instructions for optional dependencies
The added section clearly explains the purpose and installation of optional dependencies.
docs/conf.py (3)
48-48: Configuration updated to use new API parameter.
The change from jupyter_execute_notebooks to nb_execution_mode reflects an update in the underlying library's API. Both are set to "off" to maintain the same functionality.
157-157: Verify impact of commented static path.
Commenting out html_static_path might affect the inclusion of static files (CSS, images, etc.) in the documentation build.
✅ Verification successful
No impact from commented static path configuration
The verification shows that:
- No
_staticdirectory exists in the docs folder - No image or figure directives are used in the documentation
- The only reference to
_staticis in the commented configuration itself
Since there are no static assets being used in the documentation, commenting out the html_static_path configuration has no practical impact on the documentation build.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for static files that might be affected
# Test: Look for static files in the _static directory
fd -t f . 'docs/_static'
Length of output: 124
Script:
#!/bin/bash
# Check if _static directory exists and look for references to static files in docs
fd _static docs/
rg -l "_static" docs/
Length of output: 53
Script:
#!/bin/bash
# Check for any static assets referenced in RST/MD files
rg -t rst -t md -l "\.\. image::" docs/
rg -t rst -t md -l "\.\. figure::" docs/
Length of output: 84
Script:
#!/bin/bash
# Check sphinx configuration for static path settings
rg "static_path" docs/
Length of output: 68
74-76: Major version bump detected.
The version has been updated from 2.8 to 3.0, indicating significant changes. Please ensure that all breaking changes are documented in the changelog.
smact/structure_prediction/mutation.py (1)
415-415: Enhanced return type with mutation context.
The function now provides more detailed information about the mutation by including the original and new species in the returned tuple. This change improves debugging and traceability of mutations.
Also applies to: 430-430
✅ Verification successful
Return type change is self-contained and consistent
The unary_substitute method is only defined and used within the mutation.py file. The implementation correctly yields tuples matching the updated return type annotation, including the original and new species. Since this is a generator function and there are no direct consumers of this method found in the codebase, the change is self-contained and properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all consumers of unary_substitute handle the new tuple format
# Test: Search for function calls to unary_substitute
ast-grep --pattern 'unary_substitute($$$)'
Length of output: 44
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find references to unary_substitute
rg -l "unary_substitute" --type py
# Then examine the content of these files
echo "=== Files containing unary_substitute references ==="
rg "unary_substitute" --type py -B 2 -A 2
Length of output: 526
Script:
#!/bin/bash
# Let's examine the implementation and usage of unary_substitute within the same file
rg "unary_substitute" smact/structure_prediction/mutation.py -B 10 -A 20
Length of output: 1172
Script:
#!/bin/bash
# Let's examine the full implementation to see how the return values are used
rg "yield" smact/structure_prediction/mutation.py -B 15 -A 5
Length of output: 737
smact/tests/test_core.py (3)
50-51: Test updated to reflect new oxidation states structure.
The test now correctly validates both the legacy oxidation_states_smact14 and the new default oxidation_states for Rb.
339-359: Enhanced test coverage for oxidation states sets.
The test has been improved to validate smact_filter behaviour across different oxidation state sets using subtests. This provides better test isolation and clearer failure messages.
403-407: Comprehensive validation of MgB2 across oxidation state sets.
New test cases verify MgB2 validity across different oxidation state sets, ensuring consistent behaviour across the library.
smact/screening.py (3)
350-351: Documentation formatting improved.
The parameter documentation has been reformatted to improve readability.
338-338: Parameter default value updated to use newer oxidation states set.
The default value for oxidation_states_set has been changed from "smact14" to "icsd24", which is a more recent and comprehensive set of oxidation states from ICSD 2024.
Let's verify the impact of this change:
✅ Verification successful
The search results show that "smact14" is used throughout the codebase, but primarily in tests, documentation, and as a supported option rather than a default value. The change from "smact14" to "icsd24" as the default value appears to be an intentional update, as evidenced by:
- The docstring comments indicating a planned change: "In SMACT 3.0, the smact_filter function will be set to use a new default oxidation states set."
- Both "smact14" and "icsd24" remain as valid options in the codebase
- The presence of documentation and tests comparing both sets
Default oxidation states set update is properly implemented.
The change from "smact14" to "icsd24" as the default value is a deliberate improvement that maintains backwards compatibility. Users can still explicitly use "smact14" if needed, and the codebase fully supports both sets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded "smact14" references that might need updating
rg -A 2 "smact14"
Length of output: 10116
487-493: Logic updated to handle oxidation states consistently.
The conditional logic has been enhanced to:
- Handle "smact14" explicitly
- Set "icsd24" as the default when
Noneis provided - Add support for "icsd16" set
Let's verify the changes are consistent:
✅ Verification successful
Let me verify the implementation of the oxidation states handling by checking the Element class definition and its usage.
The oxidation states handling is correctly implemented and consistent.
The verification confirms that:
- The Element class properly defines all oxidation state attributes (smact14, icsd16, icsd24, and others)
- The default oxidation states are correctly set to icsd24 in the Element class
- The screening logic in screening.py correctly handles all supported oxidation state sets
- The implementation is consistent with the documentation and data loading mechanisms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent handling of oxidation states sets across the codebase
rg "oxidation_states_(smact14|icsd16|icsd24)"
Length of output: 3082
Script:
#!/bin/bash
# Check Element class definition and its attributes
ast-grep --pattern 'class Element:
$$$
'
Length of output: 11708
smact/__init__.py (2)
61-61: Documentation updated to clarify version-specific behavior.
The documentation now clearly indicates that the oxidation states source depends on the SMACT version (>3.0 uses ICSD24, <3.0 uses SMACT14).
182-182: Default oxidation states source updated to ICSD24.
The implementation now uses lookup_element_oxidation_states_icsd24 instead of the older SMACT14 set, aligning with the documented behavior for SMACT >3.0.
Let's verify the change is consistent:
✅ Verification successful
Inconsistency found in oxidation states implementation
The code shows that lookup_element_oxidation_states is still being used as the default source for oxidation_states_smact14, while lookup_element_oxidation_states_icsd24 is correctly used for the default oxidation_states. This maintains backward compatibility while implementing the new ICSD24 set as the default, which aligns with the documented behaviour.
smact/__init__.py: The Element class correctly implements both sets:- Line 182: Uses ICSD24 for default
oxidation_states - Line 186-187: Maintains SMACT14 set as
oxidation_states_smact14
- Line 182: Uses ICSD24 for default
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old oxidation states lookup
rg "lookup_element_oxidation_states\("
Length of output: 205
Script:
#!/bin/bash
# Let's check the content of data_loader.py to understand the implementation
cat smact/data_loader.py
Length of output: 31459
Script:
#!/bin/bash
# Let's check the content of __init__.py to understand the context of the change
cat smact/__init__.py
Length of output: 21147
smact/tests/test_structure.py (3)
219-221: Enhanced API key handling for Materials Project.
The test now checks both environment variable MP_API_KEY and pymatgen's PMG_MAPI_KEY setting, improving flexibility in how the API key can be provided.
Also applies to: 227-227
236-244: New test added for pymatgen structure conversion.
The test ensures that SmactStructure objects can be correctly converted to pymatgen Structure objects and validates the conversion against a reference structure.
245-252: New test added for reduced formula generation.
The test verifies that the reduced formula method correctly generates chemical formulas for both simple (NaCl) and complex (CaTiO3) structures.
docs/tutorials/smact_validity_of_GNoMe.ipynb (1)
347-347: LGTM! Consider verifying the oxidation states set.
The addition of the explicit oxidation states set parameter smact14 makes the validation more specific and controlled. The parallel processing implementation is correct for performance optimization.
✅ Verification successful
The smact14 oxidation states set is valid and well-documented
The oxidation states set smact14 is a legitimate choice as it represents the original list of oxidation states that were manually compiled for SMACT in 2014. The codebase shows that:
- It's one of the officially supported options alongside
icsd16,icsd24,pymatgen_sp, andwiki - It's well-tested and used in multiple examples and tutorials
- It's particularly recommended for reproducing Faraday Discussions results
- While it might be more restrictive than newer sets (e.g., MgB2 is invalid in
smact14but valid inicsd16), this conservative approach aligns with the tutorial's goal of testing charge-neutrality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the available oxidation states sets and their contents
# Test: Check if smact14 is a valid oxidation states set and what states it contains
# Search for oxidation states set definitions
rg -A 5 "smact14"
Length of output: 16285
| # Clean up | ||
| shutil.rmtree("data") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use temporary directories to prevent accidental data loss
Deleting the "data" directory with shutil.rmtree("data") could unintentionally remove important data if the directory path is incorrect or overlaps with existing data. It's safer to use Python's tempfile module to create and manage temporary directories within tests.
Apply this diff to utilise a temporary directory:
- save_dir = "data/binary/df_binary_label.pkl"
+ import tempfile
+ with tempfile.TemporaryDirectory() as temp_dir:
+ save_dir = os.path.join(temp_dir, "df_binary_label.pkl")
# rest of the code remains the sameCommittable suggestion skipped: line range outside the PR's diff.
| @@ -295,7 +306,7 @@ def from_py_struct(structure: pymatgen.core.Structure, determine_oxi: str = "BV" | |||
| @staticmethod | |||
| def from_mp( | |||
| species: list[tuple[str, int, int] | tuple[smact.Species, int]], | |||
| api_key: str, | |||
| api_key: str | None = None, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use compatible type annotations for broader Python support
The type union syntax str | None requires Python 3.10 or later. To maintain compatibility with earlier Python versions, use Optional[str] from the typing module.
Apply this diff to update type annotations:
+ from typing import Optional
- api_key: str | None = None,
+ api_key: Optional[str] = None,And update the docstring accordingly:
- api_key (str| None): A www.materialsproject.org API key.
+ api_key (Optional[str]): A www.materialsproject.org API key.Also applies to: 319-319
| if len(api_key) != 32: | ||
| with MPRester(api_key) as m: | ||
| structs = m.query( | ||
| criteria={"reduced_cell_formula": formula}, | ||
| properties=["structure"], | ||
| ) | ||
| except NotImplementedError: | ||
| # New API routine | ||
| docs = m.summary.search(formula=formula, fields=["structure"]) | ||
| structs = [convert_next_gen_mprest_data(doc) for doc in docs] | ||
|
|
||
| if len(structs) == 0: | ||
| raise ValueError( | ||
| "Could not find composition in Materials Project Database, " "please supply a structure." | ||
| ) | ||
| else: | ||
| # New API routine | ||
| try: | ||
| from mp_api.client import MPRester as MPResterNew | ||
|
|
||
| with MPResterNew(api_key, use_document_model=False) as m: | ||
| structs = m.materials.summary.search(formula=formula, fields=["structure"]) | ||
| except ImportError: | ||
| with MPRester(api_key) as m: | ||
| structs = m.get_structures(chemsys_formula=formula) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using API key length to determine API version
Using the length of api_key to decide between legacy and new API routines is unreliable, as API key lengths may change. Consider implementing a more robust method to detect API version compatibility.
Consider refactoring the API selection logic, perhaps by trying the new API and falling back to the legacy API if necessary, or by providing a configuration option to specify the API version.
| if api_key is None: | ||
| api_key = os.environ.get("MP_API_KEY") or SETTINGS.get("PMG_MAPI_KEY") | ||
|
|
||
| # Legacy API routine | ||
| if len(api_key) != 32: | ||
| with MPRester(api_key) as m: |
There was a problem hiding this comment.
Handle None API keys to prevent exceptions
If api_key is None, calling len(api_key) will raise a TypeError. Ensure that api_key is not None before checking its length or provide a default behaviour.
Apply this diff to fix the issue:
if api_key is None:
api_key = os.environ.get("MP_API_KEY") or SETTINGS.get("PMG_MAPI_KEY")
+ if api_key is None:
+ raise ValueError("API key is required but not provided.")
# Legacy API routine
if len(api_key) != 32:📝 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.
| if api_key is None: | |
| api_key = os.environ.get("MP_API_KEY") or SETTINGS.get("PMG_MAPI_KEY") | |
| # Legacy API routine | |
| if len(api_key) != 32: | |
| with MPRester(api_key) as m: | |
| if api_key is None: | |
| api_key = os.environ.get("MP_API_KEY") or SETTINGS.get("PMG_MAPI_KEY") | |
| if api_key is None: | |
| raise ValueError("API key is required but not provided.") | |
| # Legacy API routine | |
| if len(api_key) != 32: | |
| with MPRester(api_key) as m: |
| for si, sj in zip(self.sites.values(), other.sites.values(), strict=False): | ||
| for ci, cj in zip(si, sj, strict=False): | ||
| if not np.allclose(ci, cj, atol=1e-7): | ||
| sites_equal = False | ||
| break | ||
|
|
There was a problem hiding this comment.
Ensure compatibility with supported Python versions
The strict parameter in zip() is only available in Python 3.10 and above. If the project supports earlier Python versions, this will cause a TypeError. Consider removing the strict parameter or implementing an alternative approach to ensure compatibility.
Apply this diff to address the issue:
- for si, sj in zip(self.sites.values(), other.sites.values(), strict=False):
+ if len(self.sites) != len(other.sites):
+ return False
+ for si, sj in zip(self.sites.values(), other.sites.values()):
for ci, cj in zip(si, sj):Committable suggestion skipped: line range outside the PR's diff.
| "properties": {} | ||
| "species": [{ "element": "O", "occu": 1 }], | ||
| "abc": [0.8137989999999999, 0.4347619999999999, 0.026632], | ||
| "properties": {}, | ||
| "label": "O", | ||
| "xyz": [6.706885653513999, -1.0122213407959997, 0.11783203229599999] | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.078086, 0.480194, 0.75], | ||
| "xyz": [0.4222703473600004, 2.644634361226, 5.770509], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| "species": [{ "element": "Ti", "occu": 1 }], | ||
| "abc": [0.8507929999999999, 0.850793, 0.8507930000000001], | ||
| "properties": {}, | ||
| "label": "Ti", | ||
| "xyz": [12.348663138314, 2.6002918693279753e-16, 3.7642936412290005] | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.921914, 0.519806, 0.25], | ||
| "xyz": [4.985489652640001, 2.862794638774, 1.9235030000000004], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.421914, 0.980194, 0.75], | ||
| "xyz": [2.2816096526400007, 5.398348861226, 5.7705090000000006], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.578086, 0.01980599999999999, 0.25], | ||
| "xyz": [3.12615034736, 0.10908013877399995, 1.9235030000000002], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.790623, 0.790038, 0.9589710000000002], | ||
| "xyz": [4.27549943448, 4.351078192302, 7.378334381652002], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.20937700000000004, 0.20996199999999998, 0.45897100000000024], | ||
| "xyz": [1.1322605655200004, 1.156350807698, 3.531328381652002], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.20937700000000004, 0.20996199999999998, 0.04102899999999976], | ||
| "xyz": [1.1322605655200004, 1.156350807698, 0.3156776183479983], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.790623, 0.790038, 0.5410289999999998], | ||
| "xyz": [4.27549943448, 4.351078192302, 4.162683618347999], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.709377, 0.290038, 0.9589710000000002], | ||
| "xyz": [3.8361405655200005, 1.597363692302, 7.378334381652002], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.2906230000000001, 0.709962, 0.45897100000000024], | ||
| "xyz": [1.571619434480001, 3.910065307698, 3.5313283816520022], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.2906230000000001, 0.709962, 0.04102899999999976], | ||
| "xyz": [1.571619434480001, 3.910065307698, 0.31567761834799846], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| }, | ||
| { | ||
| "species": [{ "element": "O", "oxidation_state": -2, "occu": 1 }], | ||
| "abc": [0.709377, 0.290038, 0.5410289999999998], | ||
| "xyz": [3.8361405655200005, 1.597363692302, 4.162683618347998], | ||
| "label": "O2-", | ||
| "properties": {} | ||
| "species": [{ "element": "Ti", "occu": 1 }], | ||
| "abc": [0.14920699999999998, 0.14920699999999998, 0.149207], | ||
| "properties": {}, | ||
| "label": "Ti", | ||
| "xyz": [2.1656348616859997, 1.752656922349161e-17, 0.660159358771] |
There was a problem hiding this comment.
💡 Codebase verification
Update test files to maintain consistency with oxidation state handling
The codebase shows that oxidation states are still being used in test files and test data:
test_structure.pyreferences oxidation states in test data:"CaTiO3": [("Ca", 2, 1), ("Ti", 4, 1), ("O", -2, 3)]- Test files still expect oxidation states in structure mutations:
_mutate_structure(CaTiO3, "Ca2+", "Ba2+") - Other structure files like
Fe3O4.jsonandmp-540839_CsPbI3_oxi.jsonstill contain explicit oxidation states
Please update all test files and test data to maintain consistency with the modernised approach of removing explicit oxidation states from labels.
🔗 Analysis chain
Validate atomic positions and stoichiometry
The structure contains:
- 2 Ca atoms
- 2 Ti atoms
- 6 O atoms
This maintains the correct CaTiO3 stoichiometry. The removal of explicit oxidation states from labels is a good modernisation, but please ensure all test cases are updated accordingly.
Let me verify if there are any other test files or data files that might contain oxidation state labels.
Let me check for any other test files that might be using these structures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other test files that might expect oxidation states
# Look for test files expecting oxidation states in structure files
rg -A 2 "Ca2\+|Ti4\+|O2-" --type py "smact/tests/"
Length of output: 830
Script:
#!/bin/bash
# Check for other JSON structure files and their content
fd -e json -x cat {} \; -x echo "---" \; 2>/dev/null
# Also check for any other test files that might reference these structures
rg -l "CaTiO3|BaTiO3" "smact/tests/"
Length of output: 70525
Script:
#!/bin/bash
# Search for test files that use CaTiO3 structure
rg -A 2 "CaTiO3" "smact/tests/"
# Also check for any other structure files that might contain oxidation states
fd -e json -x grep -l "oxidation_state" {} \;
Length of output: 3630
| @@ -112,15 +111,15 @@ def get_species_occurrences_df( | |||
|
|
|||
| def write( | |||
| self, | |||
| filename: str | os.PathLike, | |||
| filename: str, | |||
There was a problem hiding this comment.
Consider maintaining PathLike support
The type hint change from str | os.PathLike to str might break existing code that passes PathLike objects. Consider keeping the broader type support or document the breaking change.
- filename: str,
+ filename: str | os.PathLike,Also applies to: 122-122
| if len(mp_api_key) != 32: | ||
| with MPRester(mp_api_key) as m: | ||
| data = m.query( | ||
| criteria={"icsd_ids.0": {"$exists": True}}, | ||
| properties=["structure", "material_id"], | ||
| ) | ||
| except NotImplementedError: | ||
| docs = m.summary.search(theoretical=False, fields=["structure", "material_id"]) | ||
| data = [convert_next_gen_mprest_data(doc) for doc in docs] | ||
| else: | ||
| from mp_api.client import MPRester as MPResterNew | ||
|
|
||
| with MPResterNew(mp_api_key, use_document_model=False) as m: | ||
| data = m.materials.summary.search(theoretical=False, fields=["structure", "material_id"]) |
There was a problem hiding this comment.
Consider using a more robust method for client selection
Using API key length to determine which client to use is fragile and may break if the API key format changes.
Consider using a version check or explicit configuration instead:
-if len(mp_api_key) != 32:
+try:
with MPRester(mp_api_key) as m:
data = m.query(
criteria={"icsd_ids.0": {"$exists": True}},
properties=["structure", "material_id"],
)
-else:
+except (AttributeError, TypeError):
from mp_api.client import MPRester as MPResterNew
with MPResterNew(mp_api_key, use_document_model=False) as m:
data = m.materials.summary.search(theoretical=False, fields=["structure", "material_id"])📝 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.
| if len(mp_api_key) != 32: | |
| with MPRester(mp_api_key) as m: | |
| data = m.query( | |
| criteria={"icsd_ids.0": {"$exists": True}}, | |
| properties=["structure", "material_id"], | |
| ) | |
| except NotImplementedError: | |
| docs = m.summary.search(theoretical=False, fields=["structure", "material_id"]) | |
| data = [convert_next_gen_mprest_data(doc) for doc in docs] | |
| else: | |
| from mp_api.client import MPRester as MPResterNew | |
| with MPResterNew(mp_api_key, use_document_model=False) as m: | |
| data = m.materials.summary.search(theoretical=False, fields=["structure", "material_id"]) | |
| try: | |
| with MPRester(mp_api_key) as m: | |
| data = m.query( | |
| criteria={"icsd_ids.0": {"$exists": True}}, | |
| properties=["structure", "material_id"], | |
| ) | |
| except (AttributeError, TypeError): | |
| from mp_api.client import MPRester as MPResterNew | |
| with MPResterNew(mp_api_key, use_document_model=False) as m: | |
| data = m.materials.summary.search(theoretical=False, fields=["structure", "material_id"]) |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Tests