ShakeNBreak v3.4
#81
ShakeNBreak v3.4
#81
Conversation
…legend entries with the same keys for cleaner plotting
…folder, or top-level folder (like in `snb-groundstate`), and update tests
… allow greater verbosity control
…ow verbosity with `True/False`), and test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
.github/workflows/lint.yml (1)
Line range hint
13-21: Consider workflow optimizations for better CI efficiency.A few suggestions to improve the workflow:
- Add pip caching to speed up dependency installation
- Consider adding Python 3.11 to the matrix to ensure backward compatibility during the transition
Here's how to implement these improvements:
strategy: max-parallel: 1 matrix: - python-version: ['3.12'] + python-version: ['3.11', '3.12'] name: Python ${{ matrix.python-version }} Test Pop steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} + cache: 'pip' + cache-dependency-path: | + setup.py + pyproject.tomlshakenbreak/plotting.py (2)
726-794: Excellent legend formatting implementation, could use more inline documentation.The function effectively handles complex legend formatting with good features:
- Merges duplicate legend entries
- Proper handler mapping for different entry types
- Consistent legend styling
Consider adding more inline comments to explain the complex logic, particularly around:
- The handler mapping logic
- The tuple handling for merged entries
- The legend entry merging algorithm
1262-1392: Well-separated plotting logic, could benefit from type hints.The functions effectively handle different plot types with good separation of concerns.
Consider adding type hints to improve code maintainability and IDE support, particularly for:
- The
colorparameter in_plot_unperturbed- The
style_settingsdictionary in_plot_distortions- The return types of both functions
tests/test_input.py (3)
260-262: Use double quotes consistently.For consistency with the codebase style, use double quotes instead of single quotes.
- 'num_distorted_neighbours_in_dimer': 2, - 'distorted_atoms_in_dimer': [[32, 'Te'], [41, 'Te']], + "num_distorted_neighbours_in_dimer": 2, + "distorted_atoms_in_dimer": [[32, "Te"], [41, "Te"]],🧰 Tools
🪛 Ruff (0.8.2)
261-261: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
262-262: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
262-262: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
262-262: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
471-474: Good error handling, minor style improvement needed.The fallback error handling for locale reset is well implemented. Consider using double quotes for consistency.
- locale.setlocale(locale.LC_CTYPE, 'C') # Fallback to a safe default + locale.setlocale(locale.LC_CTYPE, "C") # Fallback to a safe default🧰 Tools
🪛 Ruff (0.8.2)
474-474: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
2974-2980: Remove commented-out code.Commented-out code for updating test files should not be committed to the codebase. Consider moving this to a maintenance script or documentation if needed.
- # shutil.copyfile( # to update test input files (when pymatgen updates Cp2K input formats) - # "vac_1_Cd_0/Bond_Distortion_30.0%/cp2k_input.inp", - # os.path.join( - # self.CP2K_DATA_DIR, - # "vac_1_Cd_0/Bond_Distortion_30.0%/cp2k_input.inp", - # ) - # ) # most recent change was switch to lean cp2k_input.inp output, with no commentsshakenbreak/analysis.py (1)
534-570: Good performance optimization with caching!The addition of
@lru_cacheand the switch to**sm_kwargsimproves the function's performance and flexibility. The cache size of 10,000 entries seems reasonable for this use case.However, the docstring should be updated to:
- Document the cache behavior and size limit
- Warn about potential memory implications
def _cached_calculate_atomic_disp( struct1: Structure, struct2: Structure, **sm_kwargs, ) -> tuple: """ Calculate root-mean-square displacement and atomic displacements, normalized by the free length per atom ((Vol/Nsites)^(1/3)) between two structures. Should only really be used internally in ``ShakeNBreak`` as the caching in this function relies on the ``Structure`` hashes, which in ``pymatgen`` is just the composition which is unusable here, but this is monkey-patched in ``calculate_struct_comparison`` in ``shakenbreak.analysis`` for fast internal usage. + Note: + This function is cached with LRU strategy and a maximum of 10,000 entries. + For large structures, monitor memory usage as the cache may grow significantly. + Args: struct1 (:obj:`Structure`): Structure to compare to struct2. struct2 (:obj:`Structure`): Structure to compare to struct1. **sm_kwargs: Additional keyword arguments to pass to ``_scan_sm_stol_till_match`` in ``doped`` (used for ultra-fast structure matching), such as ``min_stol``, ``max_stol``, ``stol_factor`` etc..github/workflows/release.yml (1)
Line range hint
33-42: Consider enhancing version comparison logic.The current version check uses string comparison which might not handle all version formats correctly (e.g., pre-releases, dev versions).
Consider using Python's
packaging.versionfor more robust version comparison:- current_pip_version=$(pip index versions shakenbreak | grep LATEST | awk '{print $NF}') - current_repo_version=$(grep version setup.py | awk -F\" '{print $2}' | head -1) - if [ "$current_pip_version" != "$current_repo_version" ]; then + pip install packaging + python -c ' + from packaging.version import parse + import subprocess + import re + + def get_pip_version(): + cmd = "pip index versions shakenbreak" + output = subprocess.check_output(cmd.split()).decode() + latest = re.search(r"LATEST:\s*([\d.]+)", output) + return latest.group(1) if latest else "0.0.0" + + def get_repo_version(): + with open("setup.py") as f: + content = f.read() + match = re.search(r"version\s*=\s*[\"\']([\d.]+)[\"\']", content) + return match.group(1) if match else "0.0.0" + + pip_version = parse(get_pip_version()) + repo_version = parse(get_repo_version()) + needs_release = repo_version > pip_version + print(f"version_changed={str(needs_release).lower()}") + ' >> $GITHUB_ENV
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/lint.yml(1 hunks).github/workflows/pip_install_test.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/test.yml(1 hunks)shakenbreak/analysis.py(10 hunks)shakenbreak/plotting.py(29 hunks)tests/test_input.py(22 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/pip_install_test.yml
- .github/workflows/test.yml
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_input.py
261-261: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
262-262: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
262-262: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
262-262: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
474-474: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
872-872: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
914-914: f-string without any placeholders
Remove extraneous f prefix
(F541)
917-917: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
918-918: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
920-920: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
922-922: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
948-948: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
1026-1026: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2903-2903: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2918-2918: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3298-3298: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3327-3327: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3375-3375: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-14, 3.12)
- GitHub Check: build (macos-14, 3.11)
- GitHub Check: build (macos-14, 3.10)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
🔇 Additional comments (14)
.github/workflows/lint.yml (1)
19-19: LGTM! Python 3.12 upgrade looks good.The upgrade to Python 3.12 aligns with the package's modernization efforts.
shakenbreak/plotting.py (8)
Line range hint
1-21: LGTM! Well-organized imports and clear module documentation.The imports are properly organized into standard library, third-party, and local imports. The module docstring effectively communicates the purpose of the module.
623-633: Great improvement using seaborn's colorblind-friendly palette!The function now uses seaborn's "deep" color palette which is more accessible and provides better color contrast. The logic handles different cases elegantly:
- Single color: Returns turquoise as default
- 2-10 colors: Uses seaborn's deep palette
10 colors: Uses viridis colormap
709-713: Nice improvements to colorbar formatting!The changes enhance readability with:
- Better label formatting for displacement metrics
- More robust tick handling
- Consistent scientific notation
717-724: Well-implemented charge state label parsing!The function handles both numeric and string charge states elegantly, with proper formatting for positive/negative charges and good error handling.
806-806: Good addition of style customization option!The new
style_fileparameter enhances flexibility while maintaining backward compatibility.
1229-1260: Well-structured plot setup function!The function effectively centralizes plot setup logic with:
- Good error handling for font formatting
- Clear separation of concerns
- Consistent axis formatting
Line range hint
1410-1589: Good consistency in style customization across plotting functions!The changes maintain a consistent interface with other plotting functions while handling edge cases well.
1683-1823: Excellent implementation of dataset plotting with robust error handling!The function effectively handles multiple datasets with:
- Consistent style customization
- Robust error handling
- Clear warning messages
tests/test_input.py (2)
211-215: LGTM!The structure loading for dimer distortion testing follows the established pattern and is well organized.
230-234: LGTM!The structure loading for kwarg-based dimer distortion testing is consistent with the codebase patterns.
shakenbreak/analysis.py (2)
96-113: LGTM! Good improvements to distortion filename handling.The changes improve the robustness of the function by:
- Using
contextlib.suppressfor safer float conversion- Adding support for "Dimer" distortion type
- Simplifying the control flow with early returns
842-862: LGTM! Good type hint improvement.The function signature for
get_homoionic_bondshas been improved by:
- Using
Union[list[str], str]for better type safety- Maintaining backward compatibility with string input
.github/workflows/release.yml (1)
23-23: Verify Python 3.12 compatibility with all dependencies.The upgrade to Python 3.12 is part of a broader update across workflow files. Please ensure all package dependencies support Python 3.12 to prevent build failures.
Run this script to check dependency compatibility:
✅ Verification successful
Python 3.12 upgrade is safe to proceed
The dependency analysis shows that all critical packages in the project support Python 3.12, including core scientific packages and development tools. No compatibility issues were identified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check package dependencies for Python 3.12 compatibility # Get package dependencies from setup.py echo "Checking dependencies from setup.py..." python3 -c " import ast with open('setup.py', 'r') as f: tree = ast.parse(f.read()) for node in ast.walk(tree): if isinstance(node, ast.Call) and getattr(node.func, 'id', '') == 'setup': for kw in node.keywords: if kw.arg in ['install_requires', 'extras_require']: print(f'{kw.arg}:') print(ast.literal_eval(kw.value)) " # Check PyPI for each dependency's Python version support echo -e "\nChecking PyPI for Python 3.12 support..." for pkg in $(pip freeze | cut -d'=' -f1); do echo "Checking $pkg..." curl -s "https://pypi.org/pypi/$pkg/json" | \ jq -r '.info.classifiers[] | select(contains("Programming Language :: Python"))' 2>/dev/null || \ echo "No classifier data found" doneLength of output: 28496
tests/test_input.py
Outdated
| dumpfn(cls.cdte_defect_list, "cdte_defect_list.json") | ||
| return |
There was a problem hiding this comment.
Remove debug code and unreachable return statement.
This code segment appears to be debug code that was accidentally committed. The return statement makes all the code below unreachable.
- dumpfn(cls.cdte_defect_list, "cdte_defect_list.json")
- return| # use doped efficiency functions for speed (speeds up structure matching dramatically): | ||
| from doped.utils.efficiency import Composition as doped_Composition | ||
| from doped.utils.efficiency import IStructure as doped_IStructure | ||
| from doped.utils.efficiency import PeriodicSite as doped_PeriodicSite | ||
|
|
||
| Composition.__instances__ = {} | ||
| Composition.__eq__ = doped_Composition.__eq__ | ||
| PeriodicSite.__eq__ = doped_PeriodicSite.__eq__ | ||
| PeriodicSite.__hash__ = doped_PeriodicSite.__hash__ | ||
| IStructure.__instances__ = {} | ||
| IStructure.__eq__ = doped_IStructure.__eq__ | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the monkey-patching approach.
The current implementation monkey-patches several pymatgen classes for performance optimization. While this improves efficiency, it has potential risks:
- It modifies global state which could affect other parts of the codebase
- It may break if pymatgen's internal implementation changes
- It could interfere with other libraries using these classes
Consider safer alternatives:
- Create wrapper classes that inherit from the pymatgen classes
- Use composition over inheritance
- Implement a context manager to temporarily apply these optimizations
Example implementation:
from contextlib import contextmanager
@contextmanager
def optimized_structure_matching():
"""Context manager for temporary performance optimizations."""
# Store original methods
original_eq = Composition.__eq__
original_hash = PeriodicSite.__hash__
try:
# Apply optimizations
Composition.__eq__ = doped_Composition.__eq__
PeriodicSite.__hash__ = doped_PeriodicSite.__hash__
yield
finally:
# Restore original methods
Composition.__eq__ = original_eq
PeriodicSite.__hash__ = original_hashUsage:
with optimized_structure_matching():
# Your structure matching code here
result = _cached_calculate_atomic_disp(struct1, struct2, **sm_kwargs)| from typing import Optional, Union | ||
|
|
||
| import numpy as np | ||
| import pandas as pd | ||
| from doped.utils.configurations import _scan_sm_stol_till_match |
There was a problem hiding this comment.
💡 Codebase verification
Install doped>=3.0.0 package
The _scan_sm_stol_till_match function is available in doped>=3.0.0, which is correctly specified in your setup.py. However, the package is not installed in the current environment. Install it using:
pip install "doped>=3.0.0"Note: Update your requirements.txt files to match the version requirement (currently showing >=2.3.1).
🔗 Analysis chain
Verify the doped package dependency.
The import of _scan_sm_stol_till_match from doped.utils.configurations has previously caused a ModuleNotFoundError. Ensure that:
- The
dopedpackage is listed as a dependency in your project's requirements. - The correct version of
dopedis specified that contains this utility.
Run this script to check the package availability and version:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if doped package is installed and its version
pip show doped 2>/dev/null || echo "doped package not found"
# Check if the specific module exists in the installed package
python -c "from doped.utils.configurations import _scan_sm_stol_till_match" 2>/dev/null || echo "Module not found"
Length of output: 194
Script:
#!/bin/bash
# Check for dependency declarations
echo "=== Checking setup.py ==="
fd "setup.py$" --exec cat {} \; || echo "No setup.py found"
echo -e "\n=== Checking requirements files ==="
fd "requirements.*\.txt$" --exec cat {} \; || echo "No requirements.txt found"
echo -e "\n=== Checking pyproject.toml ==="
fd "pyproject.toml$" --exec cat {} \; || echo "No pyproject.toml found"
echo -e "\n=== Checking for local doped package ==="
fd "doped$" -t d || echo "No local doped directory found"
Length of output: 11525
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
shakenbreak/input.py (1)
962-966:⚠️ Potential issueHandle potential None values in auto site-matching.
The variables
auto_matching_bulk_site_indexandauto_matching_defect_site_indexfromget_defect_type_site_idxs_and_unrelaxed_structuremight remain None, which could lead to issues later in the code.Add error handling to properly manage cases when auto site-matching fails:
- ( - _defect_type, - auto_matching_bulk_site_index, - auto_matching_defect_site_index, - unrelaxed_defect_structure, - ) = get_defect_type_site_idxs_and_unrelaxed_structure(bulk_structure, defect_structure) + try: + ( + _defect_type, + auto_matching_bulk_site_index, + auto_matching_defect_site_index, + unrelaxed_defect_structure, + ) = get_defect_type_site_idxs_and_unrelaxed_structure(bulk_structure, defect_structure) + + if auto_matching_bulk_site_index is None and auto_matching_defect_site_index is None: + raise ValueError("Auto site-matching failed to identify defect sites") + + except Exception as exc: + raise ValueError("Auto site-matching failed") from exc
🧹 Nitpick comments (2)
shakenbreak/distortions.py (2)
173-174: Improve string formatting for better readability.The f-string formatting can be improved by removing unnecessary newlines and indentation.
- f"""\tDefect Site Index / Frac Coords: {site_index or np.around(frac_coords, decimals=3)} - Original Neighbour Distances: {nearest} - Distorted Neighbour Distances:\n\t{distorted}""" + f"Defect Site Index / Frac Coords: {site_index or np.around(frac_coords, decimals=3)}\n" + f"Original Neighbour Distances: {nearest}\n" + f"Distorted Neighbour Distances: {distorted}"
267-274: Improve string formatting and variable type consistency.The verbose output block could be improved for better readability and type consistency.
if verbose: - original_distance = round(struct.get_distance(site_indexes[0], site_indexes[1]), 2) + original_distance = float(round(struct.get_distance(site_indexes[0], site_indexes[1]), 2)) print( - f"""\tDefect Site Index / Frac Coords: {site_index or np.around(frac_coords, decimals=3)} - Dimer Distorted Neighbours: {distorted_atoms} - Original Distance: {original_distance} - Distorted Neighbour Distances: {2.0}""" + f"Defect Site Index / Frac Coords: {site_index or np.around(frac_coords, decimals=3)}\n" + f"Dimer Distorted Neighbours: {distorted_atoms}\n" + f"Original Distance: {original_distance:.2f}\n" + f"Distorted Neighbour Distance: 2.00" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
shakenbreak/distortions.py(5 hunks)shakenbreak/energy_lowering_distortions.py(16 hunks)shakenbreak/input.py(47 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-14, 3.12)
- GitHub Check: build (macos-14, 3.11)
- GitHub Check: build (macos-14, 3.10)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
🔇 Additional comments (13)
shakenbreak/distortions.py (2)
185-185: LGTM! Good addition of the verbose parameter.The
verboseparameter is well-documented and follows the pattern used in other functions in the module.Also applies to: 203-205
309-309: LGTM! Improved docstring clarity.The docstring for the
d_minparameter has been simplified by removing the unnecessary phrase "ignoring interstitials", making it clearer and more concise.shakenbreak/energy_lowering_distortions.py (6)
104-104: LGTM! Well-documented parameter addition for flexible structure matching.The addition of
**sm_kwargsand its documentation is clear and consistent. The parameter is properly passed through to the structure comparison function.Also applies to: 134-137, 157-157
214-214: LGTM! Consistent parameter addition across functions.The changes maintain consistency with the previous function, including proper documentation and parameter forwarding.
Also applies to: 246-249, 289-289
334-334: LGTM! Thorough implementation of flexible structure matching.The changes are well-implemented with proper documentation and consistent parameter forwarding in both structure matching calls.
Also applies to: 383-386, 495-495, 567-567
644-644: LGTM! Consistent implementation in structure comparison function.The changes maintain the established pattern with proper documentation and parameter forwarding.
Also applies to: 675-678, 710-710
969-971: LGTM! Improved warning message readability.The warning message has been reformatted for better readability while maintaining clear information presentation.
Line range hint
104-710: Excellent refactoring to improve structure matching flexibility!The changes consistently replace the fixed
stolparameter with flexible**sm_kwargsacross all relevant functions. The implementation is thorough, well-documented, and maintains a high standard of code quality.shakenbreak/input.py (5)
25-25: Verify the import statement.The import of
get_defect_type_site_idxs_and_unrelaxed_structurewas previously flagged for causing an ImportError. Please ensure this function exists in the latest version of thedopedpackage.Run the following script to verify the import:
#!/bin/bash # Description: Check if the function exists in the doped package. # Install doped package to verify pip install doped # Use python to verify the import python -c "from doped.utils.parsing import get_defect_type_site_idxs_and_unrelaxed_structure"[skip_cloning]
43-45: LGTM! Improved file path handling.The use of
MODULE_DIRfor loading default files eliminates directory traversal and makes the paths more robust.
375-382: LGTM! Robust structure matching configuration.The StructureMatcher is configured with tight tolerances and ElementComparator for accurate structure comparison:
- Small site and lattice tolerances (0.02)
- Small angle tolerance (0.5°)
- Disabled primitive cell reduction and supercell attempts
- ElementComparator for species matching
1513-1517: LGTM! Enhanced vacancy handling with Dimer distortions.The code now automatically includes "Dimer" in bond_distortions for vacancies, ensuring dimer reconstruction is considered.
Line range hint
2028-2053: LGTM! Improved verbosity control and user feedback.The code has been enhanced with better verbosity control:
- Added optional verbose parameter with clear default value
- Added detailed electron count information in verbose mode
- Added neighbor distortion information in verbose mode
Also applies to: 2065-2070, 2090-2094
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
shakenbreak/analysis.py (1)
654-665: 🛠️ Refactor suggestionConsider safer alternatives to monkey-patching.
While the performance improvements are valuable, monkey-patching pymatgen classes has risks:
- It modifies global state
- It may break with pymatgen updates
- It could affect other parts using these classes
Consider implementing the previously suggested context manager approach for safer optimization:
@contextmanager def optimized_structure_matching(): """Context manager for temporary performance optimizations.""" original_methods = { 'composition_eq': Composition.__eq__, 'site_hash': PeriodicSite.__hash__, 'site_eq': PeriodicSite.__eq__, 'structure_eq': IStructure.__eq__ } try: # Apply optimizations Composition.__eq__ = doped_Composition.__eq__ PeriodicSite.__hash__ = doped_PeriodicSite.__hash__ PeriodicSite.__eq__ = doped_PeriodicSite.__eq__ IStructure.__eq__ = doped_IStructure.__eq__ yield finally: # Restore original methods for cls, method in original_methods.items(): setattr(cls, '__eq__' if not cls.endswith('hash') else '__hash__', method)
🧹 Nitpick comments (4)
tests/test_input.py (3)
258-260: Use double quotes for string literals for consistency.Replace single quotes with double quotes in the dictionary keys:
- 'num_distorted_neighbours_in_dimer': 2, - 'distorted_atoms_in_dimer': [[32, 'Te'], [41, 'Te']], + "num_distorted_neighbours_in_dimer": 2, + "distorted_atoms_in_dimer": [[32, "Te"], [41, "Te"]],🧰 Tools
🪛 Ruff (0.8.2)
259-259: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
260-260: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
260-260: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
260-260: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
469-472: LGTM! Good error handling with fallback.The error handling is well-implemented. Consider using double quotes for string literals:
- locale.setlocale(locale.LC_CTYPE, 'C') # Fallback to a safe default + locale.setlocale(locale.LC_CTYPE, "C") # Fallback to a safe default🧰 Tools
🪛 Ruff (0.8.2)
472-472: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
911-920: Optimize string formatting in test case.Remove unnecessary f-string prefix since there are no placeholders:
- mock_print.assert_any_call(f"--Distortion Dimer") + mock_print.assert_any_call("--Distortion Dimer")🧰 Tools
🪛 Ruff (0.8.2)
912-912: f-string without any placeholders
Remove extraneous
fprefix(F541)
915-915: Use a regular
assertinstead of unittest-styleassertInReplace
assertIn(...)withassert ...(PT009)
916-916: Use a regular
assertinstead of unittest-styleassertInReplace
assertIn(...)withassert ...(PT009)
918-918: Use a regular
assertinstead of unittest-styleassertInReplace
assertIn(...)withassert ...(PT009)
920-920: Use a regular
assertinstead of unittest-styleassertInReplace
assertIn(...)withassert ...(PT009)
shakenbreak/analysis.py (1)
566-570: Consider adding a warning about the performance implications.The comment about performance bottleneck is helpful, but consider adding a warning in the docstring about the implications of using this function directly, as it relies on monkey-patched hashes.
""" + Warning + ------- + This function's caching behavior relies on monkey-patched Structure hashes. + Direct usage outside of ShakeNBreak may lead to unexpected results. + Args: struct1 (:obj:`Structure`):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
shakenbreak/analysis.py(10 hunks)tests/test_input.py(21 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_input.py
259-259: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
260-260: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
260-260: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
260-260: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
472-472: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
870-870: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
912-912: f-string without any placeholders
Remove extraneous f prefix
(F541)
915-915: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
916-916: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
918-918: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
920-920: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
946-946: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
1024-1024: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2901-2901: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2916-2916: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3296-3296: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3325-3325: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3373-3373: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-14, 3.12)
- GitHub Check: build (macos-14, 3.11)
- GitHub Check: build (macos-14, 3.10)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
🔇 Additional comments (8)
tests/test_input.py (3)
209-213: LGTM! Well-structured initialization of dimer test structure.The code follows good practices with proper path handling and consistent structure loading pattern.
228-232: LGTM! Consistent initialization of kwarged dimer test structure.Maintains the same good practices for structure loading and path handling.
Line range hint
857-870: LGTM! Well-structured test case for dimer distortion.The test case properly validates the dimer distortion functionality with appropriate assertions.
🧰 Tools
🪛 Ruff (0.8.2)
866-866: Use a regular
assertinstead of unittest-styleassertNotEqualReplace
assertNotEqual(...)withassert ...(PT009)
867-867: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
870-870: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
shakenbreak/analysis.py (5)
6-6: LGTM! Good use of imports for optimization.The new imports enhance the code's functionality:
contextlibfor error handlinglru_cachefor performance optimization_scan_sm_stol_till_matchfrom doped for efficient structure matchingAlso applies to: 11-11, 16-16
96-113: Well-structured refactoring with improved error handling!The changes improve the function by:
- Using early returns for better readability
- Adding support for Dimer distortions
- Using
contextlib.suppressfor cleaner error handling
534-535: Good use of caching for performance optimization!The
@lru_cachedecorator with a large maxsize will significantly improve performance for repeated structure comparisons.Also applies to: 538-538
842-842: Good API improvement with backward compatibility!The function now accepts both a single string and a list of elements while maintaining backward compatibility.
Also applies to: 855-857
752-762: Verify sm_kwargs propagation through function calls.The list comprehension is a good improvement, but ensure that all
sm_kwargsare correctly propagated to_scan_sm_stol_till_match.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
132-148: Use consistent list style markers.For better markdown consistency, consider using asterisks (
*) instead of dashes (-) for list items in the literature references section.🧰 Tools
🪛 LanguageTool
[uncategorized] ~146-~146: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... supercell calculations_** [Journal of Open Source Software](https://doi.org/10.21105/jos...(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.17.2)
132-132: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
134-134: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
135-135: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
136-136: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
137-137: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
138-138: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
139-139: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
140-140: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
141-141: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
142-142: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
143-143: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
144-144: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
145-145: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
146-146: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
147-147: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
148-148: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
62-64: Improve grammar in navigation instruction.Consider adding "the" before "root directory":
-2. Navigate to root directory: +2. Navigate to the root directory:🧰 Tools
🪛 LanguageTool
[uncategorized] ~62-~62: You might be missing the article “the” here.
Context: ...MTG-Bham/ShakeNBreak2. Navigate to root directory:bash cd ShakeNBreak ``` 3...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(3 hunks)docs/index.rst(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~62-~62: You might be missing the article “the” here.
Context: ...MTG-Bham/ShakeNBreak 2. Navigate to root directory:bash cd ShakeNBreak ``` 3...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~146-~146: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... supercell calculations_** [Journal of Open Source Software](https://doi.org/10.21105/jos...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.17.2)
README.md
132-132: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
134-134: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
135-135: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
136-136: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
137-137: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
138-138: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
139-139: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
140-140: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
141-141: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
142-142: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
143-143: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
144-144: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
145-145: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
146-146: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
147-147: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
148-148: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-14, 3.12)
- GitHub Check: build (macos-14, 3.11)
- GitHub Check: build (macos-14, 3.10)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
🔇 Additional comments (3)
README.md (1)
34-43: LGTM! Installation instructions are clear and helpful.The updated installation section effectively guides users by:
- Prioritizing the recommended conda installation method
- Providing pip as an alternative
- Including helpful troubleshooting resources
docs/index.rst (2)
74-87: LGTM! Installation instructions are consistent with README.md.The installation section in the documentation:
- Maintains consistency with the README
- Uses correct RST formatting
- Includes helpful troubleshooting information
216-245: LGTM! Literature references are well-formatted.The references section demonstrates:
- Proper RST formatting
- Correct use of subscripts
- Valid and accessible links
…egeneracy) as discussed
…egeneracy) as discussed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
tests/data/quantum_espresso/vac_1_Cd_0/Bond_Distortion_30.0%/espresso.pwi (1)
Line range hint
8-24: Calculation parameters are appropriate for defect study.The quantum espresso parameters are well-suited for a defect calculation:
- Large supercell (13.09 Å)
- 1x1x1 k-points adequate for this cell size
- System contains 63 atoms (31 Cd + 32 Te), indicating a vacancy
- HSE hybrid functional with spin polarization suitable for accurate electronic structure
Consider adding a comment in the input file documenting the nature of the defect (e.g., which atom was removed to create the vacancy) to improve reproducibility.
tests/test_analysis.py (1)
Line range hint
1113-1134: Improve regex pattern for ISPIN replacementThe regex pattern
r"ISPIN\s*=\s*2"could be made more robust by anchoring it to word boundaries to prevent accidental matches in comments or similar text.- r"ISPIN\s*=\s*2" + r"\bISPIN\s*=\s*2\b"🧰 Tools
🪛 Ruff (0.8.2)
1129-1129: Use a regular
assertinstead of unittest-styleassertTrueReplace
assertTrue(...)withassert ...(PT009)
shakenbreak/input.py (1)
2703-2703: Use pathlib for robust path handlingThe file path handling could be improved by using pathlib for better cross-platform compatibility.
- input_file: Optional[str] = f"{MODULE_DIR}/SnB_input_files/cp2k_input.inp", + input_file: Optional[str] = str(Path(MODULE_DIR) / "SnB_input_files" / "cp2k_input.inp"), - elif os.path.exists(f"{MODULE_DIR}/SnB_input_files/cp2k_input.inp") and not write_structures_only: + elif Path(MODULE_DIR).joinpath("SnB_input_files", "cp2k_input.inp").exists() and not write_structures_only: warnings.warn( f"Specified input file {input_file} does not exist! Using" " default CP2K input file " "(see shakenbreak/shakenbreak/cp2k_input.inp)" ) - cp2k_input = Cp2kInput.from_file(f"{MODULE_DIR}/SnB_input_files/cp2k_input.inp") + cp2k_input = Cp2kInput.from_file(str(Path(MODULE_DIR) / "SnB_input_files" / "cp2k_input.inp"))Also applies to: 2735-2741
tests/test_input.py (4)
Line range hint
49-66: Add type hints to improve code maintainability.Consider adding return type hint to improve code maintainability and IDE support.
-def _update_struct_defect_dict(defect_dict: dict, structure: Structure, poscar_comment: str) -> dict: +def _update_struct_defect_dict(defect_dict: dict, structure: Structure, poscar_comment: str) -> Dict[str, Any]:
164-164: Optimize list summation to avoid quadratic complexity.Replace quadratic list summation with functools.reduce for better performance.
- cls.cdte_defect_list = sum(list(cls.cdte_defects.values()), []) + from functools import reduce + cls.cdte_defect_list = reduce(lambda x, y: x + y, cls.cdte_defects.values(), [])🧰 Tools
🪛 Ruff (0.8.2)
164-164: Avoid quadratic list summation
Replace with
functools.reduce(RUF017)
855-855: Remove unnecessary f-string prefixes.These strings don't contain any placeholders, so the f-string prefix is unnecessary.
- mock_print.assert_any_call(f"--Distortion Dimer") + mock_print.assert_any_call("--Distortion Dimer") - f"\tDefect Site Index / Frac Coords: 1\n" + "\tDefect Site Index / Frac Coords: 1\n"Also applies to: 936-936
🧰 Tools
🪛 Ruff (0.8.2)
855-855: f-string without any placeholders
Remove extraneous
fprefix(F541)
1545-1567: Use explicit string concatenation for better readability.Replace implicit string concatenation with explicit concatenation using
+operator.- "\nDefect vac_1_Cd in charge state: -2. Number of distorted " - "neighbours: 0" + "\nDefect vac_1_Cd in charge state: -2. Number of distorted " + + "neighbours: 0" - "\nDefect vac_1_Cd in charge state: -1. Number of distorted " - "neighbours: 1" + "\nDefect vac_1_Cd in charge state: -1. Number of distorted " + + "neighbours: 1"🧰 Tools
🪛 Ruff (0.8.2)
1545-1545: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1548-1548: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1551-1551: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1555-1555: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1558-1558: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1561-1561: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1564-1564: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1567-1567: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
tests/data/vasp/CdTe/CdTe_Int_Cd_2_-60%_Distortion_NN_10_POSCAR (1)
3-5: Consider reducing coordinate precision.The atomic coordinates are specified with 16 decimal places (e.g.,
13.0867679999999993). This level of precision is excessive for atomic positions, as VASP typically uses 8-10 decimal places. Consider reducing to 8 decimal places to maintain accuracy while improving readability and reducing file size.Also applies to: 9-72
tests/test_distortions.py (1)
148-152: Consider using pytest-style assertions.The test uses unittest-style assertions (
assertEqual,assertAlmostEqual, etc.). Consider using pytest-style assertions for better readability and error messages:-self.assertEqual(output["undistorted_structure"], structure) +assert output["undistorted_structure"] == structure -self.assertAlmostEqual(min_dist, 1.06, places=2) +assert abs(min_dist - 1.06) < 0.01 -with self.assertRaises(ValueError) as e: +with pytest.raises(ValueError) as e:Also applies to: 158-158, 198-198, 234-234, 247-247
🧰 Tools
🪛 Ruff (0.8.2)
148-148: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
149-149: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
151-151: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
152-152: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
shakenbreak/distortions.py (1)
338-339: Consider using the new helper functions in apply_dimer_distortion.The function could be simplified by using the new helpers more extensively:
-input_structure_ase, defect_site_index = _get_ase_defect_structure(structure, site_index, frac_coords) -input_structure = Structure.from_ase_atoms(input_structure_ase) -sites = [d["site"] for d in cnn.get_nn_info(input_structure, defect_site_index)] +nns_to_distort, defect_site_index, input_structure_ase = _get_nns_to_distort( + structure, + num_nearest_neighbours=2, + site_index=site_index, + frac_coords=frac_coords +) +sites = [structure[nn[1]-1] for nn in nns_to_distort]Also applies to: 342-342, 344-344
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.rst(5 hunks)shakenbreak/distortions.py(18 hunks)shakenbreak/input.py(49 hunks)tests/data/castep/vac_1_Cd_0/Bond_Distortion_30.0%/castep.cell(2 hunks)tests/data/quantum_espresso/vac_1_Cd_0/Bond_Distortion_30.0%/espresso.pwi(2 hunks)tests/data/quantum_espresso/vac_1_Cd_0/Bond_Distortion_30.0%/espresso_structure.pwi(2 hunks)tests/data/quantum_espresso/vac_1_Cd_0/Bond_Distortion_30.0%/espresso_user_parameters.pwi(2 hunks)tests/data/vasp/CdTe/CdTe_Int_Cd_2_-60%_Distortion_NN_10_POSCAR(1 hunks)tests/test_analysis.py(15 hunks)tests/test_distortions.py(4 hunks)tests/test_input.py(100 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/data/quantum_espresso/vac_1_Cd_0/Bond_Distortion_30.0%/espresso_structure.pwi
- tests/data/castep/vac_1_Cd_0/Bond_Distortion_30.0%/castep.cell
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_distortions.py
148-148: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
149-149: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
151-151: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
152-152: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
158-158: Use a regular assert instead of unittest-style assertAlmostEqual
(PT009)
198-198: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
229-229: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
234-234: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
247-247: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
tests/test_input.py
164-164: Avoid quadratic list summation
Replace with functools.reduce
(RUF017)
676-676: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
731-731: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
732-732: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
733-733: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
796-796: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
797-797: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
811-811: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
812-812: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
815-815: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
855-855: f-string without any placeholders
Remove extraneous f prefix
(F541)
858-858: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
859-859: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
861-861: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
863-863: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
879-879: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
882-882: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
883-883: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
898-898: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
899-899: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
905-905: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
929-929: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
930-930: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
936-936: f-string without any placeholders
Remove extraneous f prefix
(F541)
973-973: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
974-974: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
977-977: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
998-998: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1001-1001: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
1003-1003: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1004-1004: Use a regular assert instead of unittest-style assertEqual
(PT009)
1008-1008: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1011-1011: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
1021-1021: Use a regular assert instead of unittest-style assertEqual
(PT009)
1051-1051: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
1133-1133: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1232-1232: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1233-1233: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1234-1234: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1238-1238: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1260-1260: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1272-1272: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1279-1279: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1280-1280: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1529-1529: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1545-1545: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1548-1548: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1551-1551: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1555-1555: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1558-1558: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1561-1561: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1564-1564: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1567-1567: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1644-1644: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
1661-1661: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1665-1665: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1709-1709: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1713-1713: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1915-1915: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1973-1973: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2015-2015: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2019-2019: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2047-2047: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2205-2205: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2320-2320: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2321-2321: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2322-2322: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2323-2323: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2324-2324: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2325-2325: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2408-2408: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
2409-2409: Use a regular assert instead of unittest-style assertDictEqual
(PT009)
2419-2419: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2428-2428: Use a regular assert instead of unittest-style assertDictEqual
(PT009)
2468-2468: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2469-2469: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
2515-2515: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2518-2518: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2521-2521: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2524-2524: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2527-2527: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2591-2591: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2592-2592: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
2594-2594: Use a regular assert instead of unittest-style assertDictEqual
(PT009)
2703-2703: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2723-2723: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2791-2791: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2807-2807: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
2808-2808: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2883-2883: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2889-2889: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
3054-3054: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
3056-3056: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3056-3056: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
3099-3099: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3128-3128: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3174-3174: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3175-3175: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3176-3176: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3179-3179: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
3190-3190: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3373-3373: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3374-3374: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3375-3375: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3429-3429: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3430-3430: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3433-3433: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3434-3434: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3435-3435: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3482-3482: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3483-3483: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3493-3493: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3494-3494: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3495-3495: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3542-3542: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3543-3543: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3564-3564: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3565-3565: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
3566-3566: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3572-3572: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-14, 3.12)
- GitHub Check: build (macos-14, 3.11)
- GitHub Check: build (macos-14, 3.10)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
🔇 Additional comments (15)
tests/data/quantum_espresso/vac_1_Cd_0/Bond_Distortion_30.0%/espresso_user_parameters.pwi (1)
78-78: Coordinate precision standardization improves consistency.The Te atomic positions have been adjusted with standardized precision (7 decimal places) while maintaining the symmetric positioning of the atoms. This change enhances the reproducibility of test cases for bond distortion calculations.
Let's verify the symmetry of these Te positions and their relationship to other atoms:
Also applies to: 87-87
✅ Verification successful
Verified: Te positions maintain precise symmetry across test cases
The Te atomic positions show consistent symmetrical relationships with standardized 7-decimal precision (2.1265998000) across multiple test files, confirming this is an intentional pattern for bond distortion testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze Te atom positions and their symmetry relationships # Look for similar patterns in other quantum espresso input files # Search for Te atomic positions with similar coordinates rg --no-filename "Te.*2\.126[0-9]+" tests/data/quantum_espresso -A 1 -B 1 # Check if these specific coordinates appear in other test cases rg --no-filename "2\.1265998000" tests/data/quantum_espressoLength of output: 1232
tests/data/quantum_espresso/vac_1_Cd_0/Bond_Distortion_30.0%/espresso.pwi (1)
80-80: Consistent atomic positions across input files.The Te atomic position adjustments are identical to those in
espresso_user_parameters.pwi, maintaining consistency in the atomic structure across different input files. The presence of production-level DFT parameters (HSE functional, spin polarization) in this file suggests it's the actual calculation input, whileespresso_user_parameters.pwiserves as a template.Let's verify the consistency of DFT parameters across test cases:
Also applies to: 89-89
✅ Verification successful
DFT parameter variations are intentional across test cases
The differences in DFT parameters (energy cutoffs, exchange-correlation functionals, and spin polarization) across input files are deliberate, representing distinct calculation requirements for different test cases. This is expected behavior and doesn't indicate any consistency issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare DFT parameters across quantum espresso input files # Check for HSE functional usage patterns rg --no-filename "input_dft.*=.*HSE" tests/data/quantum_espresso # Compare energy cutoffs rg --no-filename "ecutwfc.*=.*" tests/data/quantum_espresso # Look for spin polarization settings rg --no-filename "nspin.*=.*2" tests/data/quantum_espressoLength of output: 368
tests/test_analysis.py (3)
62-62: LGTM: Well-defined instance variable initializationThe new instance variable
self.dense_bond_distortionsis properly initialized with bond distortion values rounded to three decimal places, improving code maintainability by centralizing the distortion values.
79-83: LGTM: Enhanced test isolation with thorough cleanupThe tearDown method has been improved to:
- Clean up the Bond_Distortion_20.0% directory
- Recursively remove all subdirectories in v_Ca_s0_0
This ensures better test isolation and prevents test artifacts from affecting subsequent test runs.
557-559: LGTM: Improved floating-point comparisonsReplaced exact equality checks with
np.isclosefor floating-point comparisons, which is the correct approach for handling floating-point arithmetic. The atol=1e-2 tolerance is appropriate for the expected precision of these calculations.shakenbreak/input.py (4)
Line range hint
1979-1995: LGTM: Well-documented function with clear type hintsThe
_parse_distorted_elementfunction has been updated with:
- Clear type hints using Union[str, None, list[str]]
- Comprehensive docstring with Args and Returns sections
- Proper handling of multiple return types
2041-2044: LGTM: Enhanced verbosity control with informative outputThe verbosity control has been improved with:
- Clear docstring parameter description
- Informative bold-formatted output messages
- Proper handling of electron count display
Also applies to: 2056-2061
1513-1517: LGTM: Enhanced dimer distortion support for vacanciesAdded automatic inclusion of "Dimer" distortion for vacancies, ensuring dimer reconstruction is considered in the analysis. The implementation properly:
- Checks for existing dimer distortions case-insensitively
- Converts bond_distortions to list to ensure mutability
- Appends "Dimer" only if not already present
2138-2142: LGTM: Consistent dimer distortion metadata handlingThe metadata update for vacancies correctly includes the "Dimer" distortion in the bond_distortions list, maintaining consistency with the implementation.
tests/test_distortions.py (2)
127-164: Well-structured test for degenerate distances!The test case effectively verifies the handling of degenerate distances in octahedral coordination, with clear setup and assertions. The test structure and expected values are well-documented.
🧰 Tools
🪛 Ruff (0.8.2)
148-148: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
149-149: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
151-151: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
152-152: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
158-158: Use a regular
assertinstead of unittest-styleassertAlmostEqual(PT009)
228-247: Good enhancement of error handling test.The test has been improved to:
- Test multiple missing elements
- Cover various neighbor counts and distortion factors
- Use proper error handling with ValueError
🧰 Tools
🪛 Ruff (0.8.2)
229-229: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
234-234: Use
pytest.raisesinstead of unittest-styleassertRaisesReplace
assertRaiseswithpytest.raises(PT027)
247-247: Use a regular
assertinstead of unittest-styleassertInReplace
assertIn(...)withassert ...(PT009)
CHANGELOG.rst (1)
4-24: Excellent changelog entry!The v3.4.0 changelog is well-structured and comprehensive, clearly documenting:
- Major efficiency improvements with quantified benefits (">~3 orders of magnitude")
- New features with context ("Dimer" addition based on discussions)
- Behavior changes (auto-detection in snb-xxx functions)
- Technical improvements (handling of degenerate cases)
shakenbreak/distortions.py (3)
22-74: Well-structured helper function for ASE conversion!The new
_get_ase_defect_structurehelper is well-documented and handles edge cases properly. Good error message when insufficient information is provided.
76-206: Excellent implementation of nearest neighbor selection!The
_get_nns_to_distortfunction:
- Handles degenerate cases deterministically
- Provides clear warnings for edge cases
- Uses efficient sorting strategy
- Well-documented behavior
214-214: Type hint improvement for distorted_element parameter.The type hint for
distorted_elementhas been updated to support both string and list inputs:distorted_element: Optional[Union[str, list]] = NoneThis better reflects the actual usage where both single elements and lists of elements are accepted.
Also applies to: 247-249
| self.assertEqual( | ||
| len([warning for warning in w if warning.category == UserWarning]), 5 | ||
| ) | ||
| self.assertEqual(len([warning for warning in w if warning.category == UserWarning]), 5) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use isinstance() for type checking.
Replace equality operator with isinstance() for type checking.
- self.assertEqual(len([warning for warning in w if warning.category == UserWarning]), 5)
+ self.assertEqual(len([warning for warning in w if isinstance(warning.category, type(UserWarning))]), 5)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
3056-3056: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3056-3056: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
shakenbreak/distortions.py (1)
76-206: Consider simplifying the sorting logic.The function is well-implemented with good documentation and error handling. However, the sorting logic could be simplified.
Consider simplifying the sorting logic by using tuple comparison:
- nearest_neighbours = sorted( - nearest_neighbours, - key=lambda tup: ( - round(tup[0], 2), - round(input_structure_ase.get_distance(nearest_neighbour[1] - 1, tup[1] - 1, mic=True), 2), - tup[1], - ), - ) + nearest_neighbours.sort( + key=lambda tup: ( + round(tup[0], 2), + round(input_structure_ase.get_distance(nearest_neighbour[1] - 1, tup[1] - 1, mic=True), 2), + tup[1] + ) + )shakenbreak/analysis.py (1)
534-570: Consider documenting the cache size rationale.The caching and performance improvements look good. The cache size of 10,000 entries should be sufficient for most use cases, but it would be helpful to document why this specific size was chosen.
Add a comment explaining the cache size choice:
-@lru_cache(maxsize=int(1e4)) +# Cache size of 10,000 chosen based on typical number of structure comparisons needed +# for a medium-sized defect analysis project +@lru_cache(maxsize=int(1e4))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
shakenbreak/SnB_input_files/example_generate_all_config.yaml(1 hunks)shakenbreak/SnB_input_files/example_generate_config.yaml(1 hunks)shakenbreak/analysis.py(20 hunks)shakenbreak/cli.py(22 hunks)shakenbreak/distortions.py(26 hunks)shakenbreak/energy_lowering_distortions.py(23 hunks)shakenbreak/input.py(63 hunks)shakenbreak/io.py(9 hunks)shakenbreak/plotting.py(39 hunks)
✅ Files skipped from review due to trivial changes (2)
- shakenbreak/SnB_input_files/example_generate_config.yaml
- shakenbreak/SnB_input_files/example_generate_all_config.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- shakenbreak/io.py
- shakenbreak/cli.py
- shakenbreak/input.py
- shakenbreak/plotting.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-14, 3.12)
- GitHub Check: build (macos-14, 3.11)
- GitHub Check: build (macos-14, 3.10)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
🔇 Additional comments (11)
shakenbreak/distortions.py (3)
22-74: Well-structured helper function implementation!The new
_get_ase_defect_structurehelper function is well-implemented with:
- Clear type hints
- Comprehensive docstring
- Proper error handling
- Consistent handling of both defect types (substitutions/interstitials and vacancies)
214-214: Good improvements to function organization and debugging capabilities!The changes enhance the code by:
- Extracting common logic into helper functions
- Adding verbose output for better debugging
- Improving type hints accuracy
Also applies to: 247-249, 260-267, 311-311, 338-339
456-456: Good consistency improvements across the module!The changes enhance code consistency by:
- Using direct structure conversion methods
- Improving docstring formatting
- Aligning with the new helper function pattern
Also applies to: 679-682, 687-687, 699-700, 814-814, 824-824, 828-828, 859-859, 877-877, 902-902
shakenbreak/analysis.py (5)
96-113: LGTM! Clean refactoring of the distortion filename logic.The changes improve readability by:
- Reducing nested conditionals
- Using early returns
- Adding support for "Dimer" distortion type
654-665: Consider safer alternatives to monkey-patching.The current implementation monkey-patches several pymatgen classes for performance optimization. While this improves efficiency, it has potential risks:
- It modifies global state which could affect other parts of the codebase
- It may break if pymatgen's internal implementation changes
- It could interfere with other libraries using these classes
842-862: LGTM! Improved parameter flexibility and documentation.The changes enhance usability by:
- Supporting both single element and list of elements
- Providing clearer parameter documentation
Line range hint
969-971: LGTM! Improved error message clarity.The error message is now more informative and better formatted, clearly showing which files are missing and which are present.
6-6: Verify doped package dependency.The import of
_scan_sm_stol_till_matchfromdoped.utils.configurationsrequires doped>=3.0.0.Run this script to check the package availability and version:
Also applies to: 11-11, 16-16
✅ Verification successful
Dependency requirement verified: doped>=3.0.0 is correct
The codebase explicitly requires doped>=3.0.0 for the
_scan_sm_stol_till_matchfunction, which is used for optimized structure matching operations. This dependency is properly documented in setup.py and the changelog confirms its integration for significant performance improvements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if doped package is installed and its version pip show doped 2>/dev/null || echo "doped package not found" # Check if the specific module exists in the installed package python -c "from doped.utils.configurations import _scan_sm_stol_till_match" 2>/dev/null || echo "Module not found"Length of output: 194
Script:
#!/bin/bash # Check for dependency specifications echo "=== Checking setup.py ===" fd "setup.py$" --exec cat {} \; || true echo -e "\n=== Checking requirements files ===" fd "requirements.*\.txt$" --exec cat {} \; || true echo -e "\n=== Checking usage of _scan_sm_stol_till_match ===" rg "_scan_sm_stol_till_match" -A 2Length of output: 13023
shakenbreak/energy_lowering_distortions.py (3)
Line range hint
104-137: LGTM! Consistent parameter updates for structure matching.The removal of the
stolparameter in favor of**sm_kwargsaligns with the codebase-wide changes to make structure matching more flexible.
Line range hint
214-249: LGTM! Consistent parameter updates.The changes maintain consistency with the new structure matching approach.
1198-1210: LGTM! Improved docstring clarity.The documentation updates provide clearer explanation of the function's behavior and parameters.
…unding differences on GH Actions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_energy_lowering_distortions.py (1)
1030-1030: LGTM! Performance improvement using cached calculations.The switch to
_cached_calculate_atomic_dispaligns with the efficiency improvements.Minor suggestions:
- Consider breaking long lines for better readability
- Consider using plain
assertfor simpler test assertions- self.assertTrue(analysis._cached_calculate_atomic_disp(struct, self.V_Cd_minus_0pt55_structure)[0] < 0.01) + displacement = analysis._cached_calculate_atomic_disp( + struct, self.V_Cd_minus_0pt55_structure + )[0] + assert displacement < 0.01Also applies to: 1080-1080, 1129-1129, 1183-1183
🧰 Tools
🪛 Ruff (0.8.2)
1030-1030: Use a regular
assertinstead of unittest-styleassertTrueReplace
assertTrue(...)withassert ...(PT009)
1030-1030: Line too long (114 > 107)
(E501)
tests/test_input.py (2)
676-676: Consider using plain assert statements instead of unittest-style assertions.The codebase uses many unittest-style assertions like
assertEqual,assertNotEqual, etc. While these work fine, plain assert statements are generally preferred for better readability.Example transformation:
- self.assertEqual(V_Cd_distorted_dict["undistorted_structure"], self.V_Cd_struc) + assert V_Cd_distorted_dict["undistorted_structure"] == self.V_Cd_struc - self.assertNotEqual(self.V_Cd_struc, distorted_V_Cd_struc) + assert self.V_Cd_struc != distorted_V_Cd_strucAlso applies to: 731-733, 796-797, 811-812, 815-815, 858-859, 861-861, 863-863, 879-883, 898-899, 905-905, 929-930
🧰 Tools
🪛 Ruff (0.8.2)
676-676: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
164-164: Optimize list summation using functools.reduce.The current list summation could be quadratic in complexity. Using
functools.reducewould be more efficient.- cls.cdte_defect_list = sum(list(cls.cdte_defects.values()), []) + from functools import reduce + cls.cdte_defect_list = reduce(lambda x, y: x + y, cls.cdte_defects.values(), [])🧰 Tools
🪛 Ruff (0.8.2)
164-164: Avoid quadratic list summation
Replace with
functools.reduce(RUF017)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_energy_lowering_distortions.py(7 hunks)tests/test_input.py(100 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_energy_lowering_distortions.py
1030-1030: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1030-1030: Line too long (114 > 107)
(E501)
1080-1080: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1080-1080: Line too long (114 > 107)
(E501)
1129-1129: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1129-1129: Line too long (114 > 107)
(E501)
1183-1183: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1183-1183: Line too long (114 > 107)
(E501)
tests/test_input.py
164-164: Avoid quadratic list summation
Replace with functools.reduce
(RUF017)
676-676: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
731-731: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
732-732: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
733-733: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
796-796: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
797-797: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
811-811: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
812-812: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
815-815: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
855-855: f-string without any placeholders
Remove extraneous f prefix
(F541)
858-858: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
859-859: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
861-861: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
863-863: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
879-879: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
882-882: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
883-883: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
898-898: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
899-899: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
905-905: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
929-929: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
930-930: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
936-936: f-string without any placeholders
Remove extraneous f prefix
(F541)
973-973: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
974-974: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
977-977: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
998-998: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1001-1001: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
1003-1003: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1004-1004: Use a regular assert instead of unittest-style assertEqual
(PT009)
1008-1008: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1011-1011: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
1021-1021: Use a regular assert instead of unittest-style assertEqual
(PT009)
1051-1051: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
1133-1133: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1232-1232: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1233-1233: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1234-1234: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1238-1238: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1260-1260: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1272-1272: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1279-1279: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1280-1280: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1529-1529: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1545-1545: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1548-1548: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1551-1551: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1555-1555: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1558-1558: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1561-1561: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1564-1564: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1567-1567: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1644-1644: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
1661-1661: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1665-1665: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1709-1709: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1713-1713: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1915-1915: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1973-1973: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2015-2015: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2019-2019: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2047-2047: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2205-2205: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2320-2320: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2321-2321: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2322-2322: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2323-2323: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2324-2324: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2325-2325: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2352-2352: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2357-2357: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2362-2362: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2433-2433: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
2434-2434: Use a regular assert instead of unittest-style assertDictEqual
(PT009)
2444-2444: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2494-2494: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2495-2495: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
2541-2541: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2544-2544: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2547-2547: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2550-2550: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2553-2553: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2617-2617: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2618-2618: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
2620-2620: Use a regular assert instead of unittest-style assertDictEqual
(PT009)
2731-2731: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2751-2751: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2819-2819: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2835-2835: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
2836-2836: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2911-2911: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2917-2917: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
3082-3082: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
3084-3084: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3084-3084: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
3127-3127: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3156-3156: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3202-3202: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3203-3203: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3204-3204: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3207-3207: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
3218-3218: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3401-3401: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3402-3402: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3403-3403: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3457-3457: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3458-3458: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3461-3461: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3462-3462: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3463-3463: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3510-3510: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3511-3511: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3521-3521: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3522-3522: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3523-3523: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3570-3570: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3571-3571: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3592-3592: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3593-3593: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
3594-3594: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3600-3600: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-14, 3.12)
- GitHub Check: build (macos-14, 3.11)
- GitHub Check: build (macos-14, 3.10)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
🔇 Additional comments (5)
tests/test_energy_lowering_distortions.py (3)
14-25: LGTM! Performance optimization through efficient implementations.The use of
doped.utils.efficiencyimplementations improves structure matching performance. While monkey-patching core classes can be risky, it's acceptable here in the testing context.
124-124: LGTM! Improved test cleanup.The additional cleanup steps in
tearDownhelp maintain test isolation by:
- Removing generated
default_INCARfiles- Restoring original CONTCAR files
Also applies to: 135-140
Line range hint
1-1189: LGTM! Well-structured and comprehensive test coverage.The test file demonstrates excellent testing practices:
- Clear test organization and documentation
- Comprehensive coverage of edge cases
- Proper test isolation and cleanup
- Good error condition handling
🧰 Tools
🪛 Ruff (0.8.2)
27-27: Missing docstring in public function
(D103)
tests/test_input.py (2)
Line range hint
49-66: LGTM!The helper function is well-documented and correctly implements the dictionary update functionality.
430-434: Good addition of robust error handling!The try-except block with fallback to "C" locale improves reliability when the original locale cannot be restored.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
shakenbreak/plotting.py (3)
623-633: LGTM! Improved color selection logic.The refactored function provides better color selection using seaborn's "deep" palette and handles all cases elegantly. The explicit handling of different color count ranges and avoidance of hard-to-see yellow colors improves plot readability.
Consider adding a docstring example showing the color values returned for different inputs to help users understand the color selection behavior.
717-724: LGTM! Well-implemented charge state label parsing.The function handles both numeric and string charge states elegantly, with proper error handling and consistent label formatting.
Consider adding input validation to ensure
distortion_keyis a string and contains the expected format before splitting:def _parse_other_charge_state_label(distortion_key: str) -> str: + if not isinstance(distortion_key, str) or "_" not in distortion_key: + return f"From {distortion_key}" try: other_charge_state = int(distortion_key.split("_")[-1]) return f"From {'+' if other_charge_state > 0 else ''}{other_charge_state} charge state" except ValueError: other_charge_state = distortion_key.split("_")[-1] return f"From {other_charge_state}"
1262-1392: LGTM! Well-structured plotting functions with good separation of concerns.The new
_plot_unperturbedand_plot_distortionsfunctions effectively separate plotting logic and handle different cases well.Consider extracting the marker and color selection logic in
_plot_distortionsinto a separate function to improve readability:def _get_marker_for_charge_state(index: int) -> str: markers = ["s", "v", "<", ">", "^", "p", "X"] * 10 return markers[index]tests/test_input.py (4)
Line range hint
64-89: Add docstring to document the function parameters and return value.While the function is marked as from an example notebook, it would benefit from proper documentation of its parameters and return value.
Add a docstring like:
def _get_defect_entry_from_defect( # from example notebook defect, defect_supercell, charge_state, dummy_species=DummySpecies("X"), ): + """Generate DefectEntry object from Defect object and supercell. + + Args: + defect: The defect object to convert + defect_supercell: The supercell containing the defect + charge_state: Charge state of the defect + dummy_species: Dummy species used to track defect coords (default: X) + + Returns: + DefectEntry object describing the defect in the simulation cell + """🧰 Tools
🪛 Ruff (0.8.2)
79-81: Prefer
next(...)over single element sliceReplace with
next(...)(RUF015)
163-163: Optimize list summation to avoid quadratic complexity.The list summation using
sum(list(...), [])has quadratic complexity. Consider usingfunctools.reducefor better performance.- cls.cdte_defect_list = sum(list(cls.cdte_defects.values()), []) + from functools import reduce + from operator import add + cls.cdte_defect_list = reduce(add, cls.cdte_defects.values(), [])🧰 Tools
🪛 Ruff (0.8.2)
163-163: Avoid quadratic list summation
Replace with
functools.reduce(RUF017)
854-854: Remove unnecessary f-string prefixes.These strings don't contain any placeholders, so the f-string prefix is unnecessary.
- mock_print.assert_any_call(f"--Distortion Dimer") + mock_print.assert_any_call("--Distortion Dimer") - mock_print.assert_any_call(f"\tDefect Site Index / Frac Coords: 1\n" + mock_print.assert_any_call("\tDefect Site Index / Frac Coords: 1\n"Also applies to: 935-935
🧰 Tools
🪛 Ruff (0.8.2)
854-854: f-string without any placeholders
Remove extraneous
fprefix(F541)
3084-3084: Use isinstance() for type checking.Using equality operator for type checking is not recommended. Use isinstance() instead.
- self.assertEqual(len([warning for warning in w if warning.category == UserWarning]), 5) + self.assertEqual(len([warning for warning in w if isinstance(warning.category, type(UserWarning))]), 5)🧰 Tools
🪛 Ruff (0.8.2)
3084-3084: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
3084-3084: Use
isandis notfor type comparisons, orisinstance()for isinstance checks(E721)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
shakenbreak/analysis.py(20 hunks)shakenbreak/cli.py(22 hunks)shakenbreak/distortions.py(27 hunks)shakenbreak/energy_lowering_distortions.py(23 hunks)shakenbreak/input.py(64 hunks)shakenbreak/io.py(10 hunks)shakenbreak/plotting.py(39 hunks)tests/test_input.py(101 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shakenbreak/io.py
👮 Files not reviewed due to content moderation or server errors (4)
- shakenbreak/distortions.py
- shakenbreak/analysis.py
- shakenbreak/cli.py
- shakenbreak/energy_lowering_distortions.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_input.py
163-163: Avoid quadratic list summation
Replace with functools.reduce
(RUF017)
675-675: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
730-730: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
731-731: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
732-732: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
795-795: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
796-796: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
810-810: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
811-811: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
814-814: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
854-854: f-string without any placeholders
Remove extraneous f prefix
(F541)
857-857: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
858-858: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
860-860: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
862-862: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
878-878: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
881-881: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
882-882: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
897-897: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
898-898: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
904-904: Use a regular assert instead of unittest-style assertNotIn
Replace assertNotIn(...) with assert ...
(PT009)
928-928: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
929-929: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
935-935: f-string without any placeholders
Remove extraneous f prefix
(F541)
972-972: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
973-973: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
976-976: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
997-997: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1000-1000: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
1002-1002: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1003-1003: Use a regular assert instead of unittest-style assertEqual
(PT009)
1007-1007: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1010-1010: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
1020-1020: Use a regular assert instead of unittest-style assertEqual
(PT009)
1050-1050: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
1132-1132: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1231-1231: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1232-1232: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1233-1233: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1237-1237: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1259-1259: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1271-1271: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1278-1278: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1279-1279: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1528-1528: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1544-1544: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1547-1547: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1550-1550: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1554-1554: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1557-1557: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1560-1560: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1563-1563: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1566-1566: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
1643-1643: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
1660-1660: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1664-1664: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1708-1708: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1712-1712: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1914-1914: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1972-1972: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2014-2014: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2018-2018: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2046-2046: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2204-2204: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2319-2319: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2320-2320: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2321-2321: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2322-2322: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2323-2323: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2324-2324: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2351-2351: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2356-2356: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2361-2361: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2432-2432: Use a regular assert instead of unittest-style assertNotEqual
Replace assertNotEqual(...) with assert ...
(PT009)
2433-2433: Use a regular assert instead of unittest-style assertDictEqual
(PT009)
2443-2443: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2493-2493: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2494-2494: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
2540-2540: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2543-2543: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2546-2546: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2549-2549: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2552-2552: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
2616-2616: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2617-2617: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
2619-2619: Use a regular assert instead of unittest-style assertDictEqual
(PT009)
2730-2730: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2750-2750: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2818-2818: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2834-2834: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
2835-2835: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2910-2910: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2916-2916: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
3082-3082: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
3084-3084: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3084-3084: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
3127-3127: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3156-3156: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3202-3202: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3203-3203: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3204-3204: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3207-3207: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
3218-3218: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3401-3401: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3402-3402: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3403-3403: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3457-3457: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3458-3458: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3461-3461: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3462-3462: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3463-3463: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3510-3510: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3511-3511: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3521-3521: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
3522-3522: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3523-3523: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3570-3570: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3571-3571: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
3592-3592: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3593-3593: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
3594-3594: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3600-3600: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-14, 3.12)
- GitHub Check: build (macos-14, 3.11)
- GitHub Check: build (macos-14, 3.10)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
🔇 Additional comments (20)
shakenbreak/plotting.py (7)
Line range hint
1-21: LGTM! Well-organized imports and clear module documentation.The imports are properly organized and the module's purpose is well documented. The addition of
HandlerTupleandPathLikeimproves legend handling and type safety.
709-713: LGTM! Enhanced colorbar formatting.The changes improve the colorbar's appearance with better label formatting and more robust tick handling. The conditional label formatting based on the metric type is particularly well done.
726-794: LGTM! Excellent legend formatting implementation.The new function provides sophisticated legend handling with:
- Smart merging of line and point handles
- Efficient deduplication of legend entries
- Proper spacing and layout management
- Good use of HandlerTuple for complex legend entries
806-806: LGTM! Well-implemented style customization support.The addition of the
style_fileparameter across plotting functions provides consistent and flexible plot styling capabilities while maintaining good defaults.Also applies to: 847-849, 1007-1007, 1061-1063, 1478-1480, 1643-1645
1229-1259: LGTM! Excellent refactoring of plot setup logic.The new
_setup_plotfunction effectively centralizes common plot setup tasks, reducing code duplication and improving maintainability. The error handling for defect name formatting is particularly well done.
1395-1409: LGTM! Well-implemented x-axis limit handling.The
_set_xlimfunction provides robust x-axis limit calculation with proper padding and edge case handling. The use of numpy's array operations makes the code efficient.
1683-1823: LGTM! Excellent improvements to plot_datasets.The function has been well refactored to:
- Use the new helper functions effectively
- Handle styles more robustly
- Validate inputs properly
- Provide clear error messages
shakenbreak/input.py (10)
1513-1517: LGTM! Good enhancement for vacancy handling.The addition of "Dimer" to bond_distortions for vacancies ensures that dimer reconstruction possibilities are always explored, which is important for accurate vacancy defect modeling.
2474-2488: LGTM! Improved documentation clarity.The docstring updates provide more accurate paths to configuration files, making it easier for users to locate and review the default settings.
2584-2591: LGTM! Enhanced documentation for QE input settings.The docstring updates provide clearer references to the QE input configuration files and better explain the relationship between
input_parametersandinput_file.
Line range hint
2999-3011: LGTM! Better explanation of charge state determination.The docstring updates provide a clearer explanation of how the charge state range is calculated using the defect oxidation state and padding parameter.
Line range hint
1979-1995: LGTM! Enhanced type documentation.The docstring updates provide more precise return type information using Union[str, None, list[str]], which helps with type checking and IDE support.
2081-2085: LGTM! Improved user feedback.The added verbose output provides helpful information about the defect charge state and number of distorted neighbors, which aids in debugging and understanding the distortion process.
1666-1682: LGTM! Improved documentation and backward compatibility.The changes:
- Provide clearer documentation about the expected format of defect_entries
- Add support for older versions of the doped package (< 2.0)
- Include helpful links to the doped documentation
These improvements make the code more maintainable and user-friendly.
Line range hint
289-321: LGTM! Improved function return value.The function now returns the final defect folder name, which is helpful when the folder name is modified to avoid duplicates. This change makes it easier for callers to track where files are written.
25-25: Verify the import is now available.A previous review flagged that
get_defect_type_site_idxs_and_unrelaxed_structurecouldn't be imported fromdoped.utils.parsing. Please verify this function is now available in the latest version of thedopedpackage.✅ Verification successful
Import is now available and working correctly
The function
get_defect_type_site_idxs_and_unrelaxed_structureis properly available in thedoped.utils.parsingmodule and can be imported without any issues. The previous ImportError has been resolved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the function exists in the doped package. # Install doped package to verify pip install doped # Use Python to verify the import python -c "from doped.utils.parsing import get_defect_type_site_idxs_and_unrelaxed_structure"Length of output: 18513
Script:
#!/bin/bash # Description: Verify function existence and its module path using Python's inspect module python3 -c " from importlib import import_module import inspect try: module = import_module('doped.utils.parsing') func = getattr(module, 'get_defect_type_site_idxs_and_unrelaxed_structure', None) if func is not None and inspect.isfunction(func): print(f'Function exists at: {module.__name__}.{func.__name__}') print(f'Function signature: {inspect.signature(func)}') else: print('Function does not exist in the module') except ImportError as e: print(f'Import error: {str(e)}') except Exception as e: print(f'Error: {str(e)}') "Length of output: 1078
962-966: Verify API compatibility with doped package.The code now uses
get_defect_type_site_idxs_and_unrelaxed_structurefrom doped.utils.parsing. Please ensure this change is compatible with the minimum required version of the doped package.✅ Verification successful
API compatibility with doped package is verified
The function
get_defect_type_site_idxs_and_unrelaxed_structureexists in doped v3.0.0 with a matching signature and return values. The code's usage is correct and follows the documented API.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the minimum required version of doped package. # Install doped package to verify pip install doped # Use Python to check the function signature python -c "from doped.utils.parsing import get_defect_type_site_idxs_and_unrelaxed_structure; import inspect; print(inspect.signature(get_defect_type_site_idxs_and_unrelaxed_structure))"Length of output: 18516
Script:
#!/bin/bash # Description: Verify the existence and structure of doped.utils.parsing module # List all files in doped.utils python -c "import doped.utils; import pkgutil; print('Available modules in doped.utils:'); [print(m.name) for m in pkgutil.iter_modules(doped.utils.__path__)]" # Try to import the specific module and list its contents python -c "from doped.utils import parsing; print('\nContents of doped.utils.parsing:'); [print(item) for item in dir(parsing) if not item.startswith('_')]"Length of output: 1721
Script:
#!/bin/bash # Description: Inspect the function signature and docstring python -c "from doped.utils.parsing import get_defect_type_site_idxs_and_unrelaxed_structure; import inspect; print('Function signature:'); print(inspect.signature(get_defect_type_site_idxs_and_unrelaxed_structure)); print('\nDocstring:'); print(inspect.getdoc(get_defect_type_site_idxs_and_unrelaxed_structure))"Length of output: 1959
tests/test_input.py (3)
Line range hint
48-63: LGTM! Well-structured helper function.The function is well-documented and handles dictionary updates safely using deepcopy.
429-433: LGTM! Good error handling for locale reset.The try-except block with fallback to a safe default locale is a good practice for cleanup.
988-1007: LGTM! Comprehensive test coverage for dimer distortions.The test method thoroughly validates dimer distortion functionality including parameters and structure validation.
🧰 Tools
🪛 Ruff (0.8.2)
997-997: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
1000-1000: Use a regular
assertinstead of unittest-styleassertNotEqualReplace
assertNotEqual(...)withassert ...(PT009)
1002-1002: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
1003-1003: Use a regular
assertinstead of unittest-styleassertEqual(PT009)
1007-1007: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (3)
shakenbreak/analysis.py (3)
98-98: Clarify the purpose of adding zero.The expression
round(distortion * 100, 1)+0adds zero, which seems unnecessary. If this is meant to avoid negative zero (-0.0), consider usingabs()or document the intention.- return f"Bond_Distortion_{round(distortion * 100, 1)+0}%" + # Avoid negative zero in the filename + percentage = round(distortion * 100, 1) + return f"Bond_Distortion_{percentage}%"
109-111: Improve error handling in float conversion.Using
contextlib.suppress(Exception)silently ignores all exceptions. This could hide unexpected errors.- with contextlib.suppress(Exception): # try converting to float, in case user entered '0.5' - distortion = float(distortion) - return f"Bond_Distortion_{round(distortion * 100, 1)+0}%" + try: + # Try converting to float, in case user entered '0.5' + distortion = float(distortion) + percentage = round(distortion * 100, 1) + return f"Bond_Distortion_{percentage}%" + except ValueError: + # Only suppress ValueError from float conversion + pass
855-857: Fix typo in docstring.There's a typo in the docstring: "wihout" should be "without".
- List or single string of element symbols (wihout + List or single string of element symbols (without
🛑 Comments failed to post (1)
shakenbreak/analysis.py (1)
534-570:
⚠️ Potential issueReconsider the caching strategy.
The
@lru_cachedecorator's effectiveness relies on the hash ofStructureobjects, but as noted in the docstring, pymatgen's default hashing only considers composition. This could lead to cache collisions or incorrect results.Consider these alternatives:
- Implement a custom caching decorator that uses a more reliable hash combining structure parameters
- Use a caching strategy that doesn't rely on object hashing
- If sticking with
lru_cache, add runtime validation to detect cache collisionsExample implementation of a custom caching decorator:
from functools import wraps from typing import Any, Callable import hashlib def structure_cache(maxsize: int = int(1e4)): cache = {} def decorator(func: Callable) -> Callable: @wraps(func) def wrapper(struct1: Structure, struct2: Structure, **kwargs) -> Any: # Create a more reliable hash combining lattice, sites, and species def get_structure_hash(struct: Structure) -> str: m = hashlib.sha256() # Include lattice parameters m.update(str(struct.lattice.matrix).encode()) # Include fractional coordinates and species for site in struct: m.update(str(site.frac_coords).encode()) m.update(str(site.species).encode()) return m.hexdigest() key = (get_structure_hash(struct1), get_structure_hash(struct2), frozenset(kwargs.items())) if key not in cache: if len(cache) >= maxsize: # Implement LRU eviction if needed cache.clear() cache[key] = func(struct1, struct2, **kwargs) return cache[key] return wrapper return decorator
|
All tests now passing, merging |
Just to help track latest updates, to be merged and released soon
Updates:
dopedupdates.Dimerto default generated distortion set.snb-xxxrobustness updates.To be released once the next
dopedrelease is ready -- hopefully soon...Remaining TODO: Prune the git history of old, unused files to reduce storage space.