v3.4.2
#86
v3.4.2
#86
Conversation
WalkthroughThis set of changes introduces new documentation and workflow resources for handling polaronic distortions in materials using ShakeNBreak, including a new Jupyter tutorial and a helper script for identifying hole states in VASP calculations. The core codebase is updated to improve neighbor-finding and distortion application logic, particularly enhancing support for complex, low-symmetry structures by using Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Distortions (shakenbreak.input)
participant tqdm
participant distortions.py
User->>Distortions: apply_distortions()
Distortions->>tqdm: Wrap defects_dict.items() iteration
loop For each defect
Distortions->>distortions.py: apply_dimer_distortion()
distortions.py->>distortions.py: CrystalNN finds neighbors
alt <2 neighbors found
distortions.py->>distortions.py: _get_nns_to_distort() fallback
distortions.py->>distortions.py: Use PeriodicNeighbor for neighbors
end
distortions.py-->>Distortions: Return distorted structure
end
Distortions-->>User: Distorted structures with progress feedback
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (13)
docs/hole-finder.py (4)
1-7: Missing docstring for the module.While the imports are appropriate for the functionality, the module is missing a docstring that would explain its purpose and usage.
Add a module-level docstring that explains what this script does and how to use it:
+""" +Helper script to identify hole states in VASP defect calculations. + +This script analyzes the vasprun.xml output from a VASP calculation to locate +single hole states in the electronic structure, which is useful for polaron studies. +""" from pymatgen.io.vasp.outputs import BSVasprun from pymatgen.electronic_structure.core import Spin import pandas as pd import warnings warnings.filterwarnings("ignore", message="No POTCAR file with matching TITEL fields")🧰 Tools
🪛 Ruff (0.8.2)
1-1: Missing docstring in public module
(D100)
38-40: Use a more descriptive DataFrame variable name.The generic variable name 'df' for DataFrames is discouraged as it lacks descriptive context.
Use a more descriptive variable name for the DataFrame:
- df = pd.DataFrame(eigenvals, + eigenval_df = pd.DataFrame(eigenvals, columns=["band", "k-point", "up-eigenval", "down-eigenval", "up-occupancy", "down-occupancy"])🧰 Tools
🪛 Ruff (0.8.2)
38-38: Avoid using the generic variable name
dffor DataFrames(PD901)
54-64: Boolean comparisons can be simplified.The comparisons with
== Trueand== Falseare redundant and can be simplified. Additionally, the logic for handling the two possible hole spin channels could be more concise.Simplify the boolean comparisons and refactor the conditional logic:
- if hole_spin_up.empty == True and hole_spin_down.empty == False: + if hole_spin_up.empty and not hole_spin_down.empty: print("Hole is in the spin down channel:\n", hole_spin_down) print("Recommended min and max for EINT in PARCHG calculation:", round(hole_spin_down["down-eigenval"].min(), 2), round(hole_spin_down["down-eigenval"].max(), 2)) - elif hole_spin_down.empty == True and hole_spin_up.empty == False: + elif hole_spin_down.empty and not hole_spin_up.empty: print("Hole is in the spin up channel:\n", hole_spin_up) print("Recommended min and max for EINT in PARCHG calculation:", round(hole_spin_up["down-eigenval"].min(), 2), round(hole_spin_up["down-eigenval"].max(), 2)) - elif hole_spin_up.empty == True and hole_spin_down.empty == True: + elif hole_spin_up.empty and hole_spin_down.empty: print("No holes found!")🧰 Tools
🪛 Ruff (0.8.2)
54-54: Avoid equality comparisons to
True; useif hole_spin_up.empty:for truth checksReplace with
hole_spin_up.empty(E712)
54-54: Avoid equality comparisons to
False; useif not hole_spin_down.empty:for false checksReplace with
not hole_spin_down.empty(E712)
59-59: Avoid equality comparisons to
True; useif hole_spin_down.empty:for truth checksReplace with
hole_spin_down.empty(E712)
59-59: Avoid equality comparisons to
False; useif not hole_spin_up.empty:for false checksReplace with
not hole_spin_up.empty(E712)
63-63: Avoid equality comparisons to
True; useif hole_spin_up.empty:for truth checksReplace with
hole_spin_up.empty(E712)
63-63: Avoid equality comparisons to
True; useif hole_spin_down.empty:for truth checksReplace with
hole_spin_down.empty(E712)
66-67: Main guard implementation is straightforward.The main guard uses a default filename that users would need to modify. Consider adding argparse support for better command-line usage.
Add command-line argument parsing for better usability:
+import argparse if __name__ == "__main__": - hole_finder("vasprun.xml") # insert your vasprun.xml file here + parser = argparse.ArgumentParser(description="Find holes in VASP electronic structure") + parser.add_argument("vasprun", nargs="?", default="vasprun.xml", + help="Path to vasprun.xml file (default: vasprun.xml)") + parser.add_argument("--bands-below", type=int, default=3, + help="Number of bands below VBM to analyze (default: 3)") + parser.add_argument("--bands-above", type=int, default=3, + help="Number of bands above VBM to analyze (default: 3)") + args = parser.parse_args() + + hole_finder(args.vasprun, args.bands_below, args.bands_above)docs/ShakeNBreak_Example_Workflow.ipynb (1)
37-42: Informative tip addition looks good
The new{tip}block effectively highlights the polaron workflow tutorial and broadens the use cases for ShakeNBreak.Nitpick: to keep tutorial naming consistent across the docs, consider renaming the link text from “SnB Polarons Tutorial” to “ShakeNBreak Polaron Workflow Tutorial” (or similar).
README.md (1)
90-97: Update publication list with recent papers using ShakeNBreakThe list of studies using ShakeNBreak has been expanded with recent publications, which is valuable for showcasing the tool's impact and providing reference materials for users.
For consistency with the rest of the list, consider using asterisks instead of dashes for these new bullet points to follow the markdownlint style guideline (MD004):
- - Y\. Liu **_Small hole polarons in yellow phase δ-CsPbI<sub>3</sub>_** [_arXiv_](https://doi.org/10.48550/arXiv.2501.16695) 2025 - - S. R. Kavanagh et al. **_Intrinsic point defect tolerance in selenium for indoor and tandem photovoltaics_** [_Energy & Environmental Science_](https://doi.org/10.1039/D4EE04647A) 2025 - - J. Huang et al. **_Manganese in β-Ga<sub>2</sub>O<sub>3</sub>: a deep acceptor with a large nonradiative electron capture cross-section_** [_Journal of Physics D: Applied Physics_](https://doi.org/10.1088/1361-6463/adca42) 2025 - - R. Desai et al. **_Exploring the Defect Landscape and Dopability of Chalcogenide Perovskite BaZrS<sub>3</sub>_** [_Journal of Physical Chemistry C_](https://doi.org/10.1021/acs.jpcc.5c01597) 2025 - - C. Kaewmeechai, J. Strand & A. Shluger **_Structure and Migration Mechanisms of Oxygen Interstitial Defects in β-Ga<sub>2</sub>O<sub>3</sub>_** [_Physica Status Solidi B_](https://onlinelibrary.wiley.com/doi/10.1002/pssb.202400652) 2025 <!-- though didn't cite properly SnB or doped code papers... --> + * Y\. Liu **_Small hole polarons in yellow phase δ-CsPbI<sub>3</sub>_** [_arXiv_](https://doi.org/10.48550/arXiv.2501.16695) 2025 + * S. R. Kavanagh et al. **_Intrinsic point defect tolerance in selenium for indoor and tandem photovoltaics_** [_Energy & Environmental Science_](https://doi.org/10.1039/D4EE04647A) 2025 + * J. Huang et al. **_Manganese in β-Ga<sub>2</sub>O<sub>3</sub>: a deep acceptor with a large nonradiative electron capture cross-section_** [_Journal of Physics D: Applied Physics_](https://doi.org/10.1088/1361-6463/adca42) 2025 + * R. Desai et al. **_Exploring the Defect Landscape and Dopability of Chalcogenide Perovskite BaZrS<sub>3</sub>_** [_Journal of Physical Chemistry C_](https://doi.org/10.1021/acs.jpcc.5c01597) 2025 + * C. Kaewmeechai, J. Strand & A. Shluger **_Structure and Migration Mechanisms of Oxygen Interstitial Defects in β-Ga<sub>2</sub>O<sub>3</sub>_** [_Physica Status Solidi B_](https://onlinelibrary.wiley.com/doi/10.1002/pssb.202400652) 2025 <!-- though didn't cite properly SnB or doped code papers... -->🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
90-90: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
91-91: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
92-92: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
93-93: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
94-94: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
95-95: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
96-96: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
97-97: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
tests/test_distortions.py (2)
89-89: Load new test data for interface structureThe test now loads a silicon vacancy defect in a HfSiO interface structure, which will be used to test the fallback behavior.
The line exceeds the recommended line length (118 > 107). Consider splitting it for better readability:
- self.V_Si_HfSiO_interface_entry = loadfn(os.path.join(self.DATA_DIR, "v_Si_C1_O1.70_HfSiO_interface_+1.json")) + self.V_Si_HfSiO_interface_entry = loadfn( + os.path.join(self.DATA_DIR, "v_Si_C1_O1.70_HfSiO_interface_+1.json") + )🧰 Tools
🪛 Ruff (0.8.2)
89-89: Line too long (118 > 107)
(E501)
424-442: New test for dimer distortion fallback behaviorThis test addresses the bug reported in issue #85, verifying that the
apply_dimer_distortionfunction correctly falls back to using_get_nns_to_distortwhenCrystalNNreturns fewer than two neighbors. This is crucial for handling complex, low-symmetry structures where neighbor-finding can be challenging.Consider addressing the code style issues reported by the linter:
- self.assertEqual(output["undistorted_structure"], self.V_Si_HfSiO_interface_entry.defect_supercell) - self.assertEqual(output["num_distorted_neighbours"], 2) - self.assertTrue(output["distorted_atoms"] == [[82, 'Si'], [177, 'O']]) - self.assertTrue(min_dist > 1.5) # Si-O dimer bond length is larger than min_dist in orig structure + assert output["undistorted_structure"] == self.V_Si_HfSiO_interface_entry.defect_supercell + assert output["num_distorted_neighbours"] == 2 + assert output["distorted_atoms"] == [[82, "Si"], [177, "O"]] + assert min_dist > 1.5 # Si-O dimer bond length is larger than min_dist in orig structure🧰 Tools
🪛 Ruff (0.8.2)
436-436: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
437-437: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
438-438: Use a regular
assertinstead of unittest-styleassertTrueReplace
assertTrue(...)withassert ...(PT009)
438-438: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
438-438: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
442-442: Use a regular
assertinstead of unittest-styleassertTrueReplace
assertTrue(...)withassert ...(PT009)
docs/ShakeNBreak_Polaron_Workflow.ipynb (5)
65-70: Consider adding file path validation for better error handling.The code loads the structure from a file without checking if it exists. For a more robust tutorial, consider adding a file existence check and appropriate error handling to guide users if the file is missing.
-bulk_supercell = Structure.from_file("CuSbSe2_POSCAR") # load our bulk supercell structure +import os + +poscar_path = "CuSbSe2_POSCAR" +if not os.path.exists(poscar_path): + raise FileNotFoundError(f"Structure file {poscar_path} not found. Please ensure it's in the same directory as this notebook.") + +bulk_supercell = Structure.from_file(poscar_path) # load our bulk supercell structure
257-259: Consider clarifying the PeriodicSite tolerance value choice.The code sets a specific tolerance value for position matching without explaining why 1e-3 was chosen. Adding a brief comment would help users understand when they might need to adjust this value.
-PeriodicSite.position_atol = 1e-3 # update position tol to ensure site-matching works +# Set position tolerance to 1e-3 Angstroms to ensure reliable site-matching +# between structures with slight position differences after symmetry analysis +PeriodicSite.position_atol = 1e-3
325-336: Consider refactoring to avoid code duplication.The two update blocks perform very similar operations. Consider using a function or loop to reduce duplication.
-trial_hole_polaron_supercells.update({f"Se_Cs_Cu2.41_{distortion_factor:+.1%}": distort_and_rattle( - structure=bulk_supercell, - site_index=Se_Cs_Cu2pt41_site_idx, - num_nearest_neighbours=4, # let's distort the 4 nearest neighbours - distortion_factor=distortion_factor, -)[\"distorted_structure\"] for distortion_factor in [-0.4, 0.4]}) -trial_hole_polaron_supercells.update({f"Se_Cs_Cu2.44_{distortion_factor:+.1%}": distort_and_rattle( - structure=bulk_supercell, - site_index=Se_Cs_Cu2pt44_site_idx, - num_nearest_neighbours=4, # let's distort the 4 nearest neighbours - distortion_factor=distortion_factor, -)[\"distorted_structure\"] for distortion_factor in [-0.4, 0.4]}) +# Create distortion for each Se site type +se_sites = { + "Se_Cs_Cu2.41": Se_Cs_Cu2pt41_site_idx, + "Se_Cs_Cu2.44": Se_Cs_Cu2pt44_site_idx +} + +for site_name, site_idx in se_sites.items(): + trial_hole_polaron_supercells.update({ + f"{site_name}_{distortion_factor:+.1%}": distort_and_rattle( + structure=bulk_supercell, + site_index=site_idx, + num_nearest_neighbours=4, # let's distort the 4 nearest neighbours + distortion_factor=distortion_factor, + )["distorted_structure"] + for distortion_factor in [-0.4, 0.4] + })
357-358: Add error handling for file writing operations.Consider adding error handling to catch potential issues when writing files.
-for name, structure in trial_hole_polaron_supercells.items(): - structure.to(fmt="POSCAR", filename=f"{name}_POSCAR") +import os + +for name, structure in trial_hole_polaron_supercells.items(): + filename = f"{name}_POSCAR" + try: + structure.to(fmt="POSCAR", filename=filename) + print(f"Successfully wrote {filename}") + except Exception as e: + print(f"Error writing {filename}: {e}")
372-373: Consider using Python for file listing instead of shell command.For better cross-platform compatibility, consider using Python's os.listdir() or glob module instead of the shell command.
-!ls *_POSCAR # check the POSCARs have been written +import glob +poscar_files = glob.glob("*_POSCAR") +print("\n".join(sorted(poscar_files))) # check the POSCARs have been written
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(2 hunks)docs/CuSbSe2_POSCAR(1 hunks)docs/ShakeNBreak_Example_Workflow.ipynb(1 hunks)docs/ShakeNBreak_Polaron_Workflow.ipynb(1 hunks)docs/Tutorials.rst(1 hunks)docs/hole-finder.py(1 hunks)docs/index.rst(2 hunks)shakenbreak/distortions.py(3 hunks)shakenbreak/input.py(2 hunks)tests/test_distortions.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_distortions.py (1)
shakenbreak/distortions.py (1)
apply_dimer_distortion(351-478)
🪛 Ruff (0.8.2)
docs/hole-finder.py
1-1: Missing docstring in public module
(D100)
9-9: Missing docstring in public function
(D103)
38-38: Avoid using the generic variable name df for DataFrames
(PD901)
47-47: Line too long (118 > 107)
(E501)
49-49: Line too long (108 > 107)
(E501)
54-54: Avoid equality comparisons to True; use if hole_spin_up.empty: for truth checks
Replace with hole_spin_up.empty
(E712)
54-54: Avoid equality comparisons to False; use if not hole_spin_down.empty: for false checks
Replace with not hole_spin_down.empty
(E712)
59-59: Avoid equality comparisons to True; use if hole_spin_down.empty: for truth checks
Replace with hole_spin_down.empty
(E712)
59-59: Avoid equality comparisons to False; use if not hole_spin_up.empty: for false checks
Replace with not hole_spin_up.empty
(E712)
63-63: Avoid equality comparisons to True; use if hole_spin_up.empty: for truth checks
Replace with hole_spin_up.empty
(E712)
63-63: Avoid equality comparisons to True; use if hole_spin_down.empty: for truth checks
Replace with hole_spin_down.empty
(E712)
tests/test_distortions.py
89-89: Line too long (118 > 107)
(E501)
436-436: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
437-437: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
438-438: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
438-438: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
438-438: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
442-442: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
🪛 markdownlint-cli2 (0.17.2)
README.md
90-90: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
91-91: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
92-92: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
93-93: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
94-94: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
95-95: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
96-96: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
97-97: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🪛 GitHub Actions: Linting
shakenbreak/input.py
[error] 2705-2705: Ruff E501: Line too long (119 > 107).
⏰ 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 (18)
shakenbreak/input.py (2)
38-39: Appropriately added tqdm import for progress bar functionality.The import of tqdm is added at the correct location with the other third-party imports, maintaining the organized import structure of the file.
2338-2342: Progress bar addition improves user experience.Adding a progress bar using tqdm provides helpful feedback to users during potentially long-running distortion generation steps. The implementation includes percentage completion, elapsed time, and processing rate, which helps users monitor the execution progress.
docs/hole-finder.py (1)
51-52: Concise but effective filtering to identify hole states.The approach to identify holes by filtering for specific occupancy patterns (1.0 in one spin channel, 0.0 in the other) is effective.
docs/CuSbSe2_POSCAR (1)
1-72: Well-structured POSCAR file with oxidation states.The POSCAR file provides a complete and well-formatted crystal structure for CuSbSe2, including explicit oxidation states for Cu+, Sb3+, and Se2-. The structure appears to be a supercell with proper lattice vectors and atomic positions in direct coordinates. All necessary components are included for using this file in VASP calculations.
docs/Tutorials.rst (2)
9-10: Added link to new polaron tutorial.Documentation has been appropriately updated to include a link to the new polaron tutorial alongside the existing python API tutorial. The links are properly formatted with full URLs.
17-17: Added polaron workflow to toctree.The toctree directive has been updated to include the new ShakeNBreak_Polaron_Workflow notebook, ensuring it will be properly indexed and accessible in the documentation.
README.md (1)
61-61: URL improvement for Python API tutorialThe link to the Python API tutorial has been updated to point directly to the documentation site, which is more user-friendly.
docs/index.rst (2)
110-113: Added link to new polarons tutorialThe Python API documentation has been enhanced by adding a reference to the new polarons tutorial, which expands the documented capabilities of ShakeNBreak to include polaron workflows.
152-159: Consistent updates to publication listThe list of studies has been updated to mirror the changes in the README, maintaining documentation consistency across files.
tests/test_distortions.py (1)
1-3: Helpful module docstring additionAdding a module-level docstring helps clarify test coverage and indicates that most distortion functions are already implicitly tested elsewhere.
shakenbreak/distortions.py (3)
13-13: Add import for PeriodicNeighbor classAdded the
PeriodicNeighborimport which is needed for the improved neighbor object construction in the fallback mechanism.
168-177: Performance improvement in neighbor distance calculationImproved the efficiency of neighbor distance calculations by using a single batch call to
get_distances()instead of computing distances repeatedly inside a list comprehension.
415-425: Enhanced fallback logic for dimer distortionImproved the robustness of the
apply_dimer_distortionfunction whenCrystalNNfails to find enough neighbors by creating properPeriodicNeighborobjects with precomputed distances. This directly addresses the issue reported in #85, ensuring that complex structures with challenging neighbor-finding scenarios are handled correctly.docs/ShakeNBreak_Polaron_Workflow.ipynb (5)
96-102: LGTM! Well-documented code example for atomic rattling.The code demonstrates the rattling functionality well, with helpful comments about additional options.
134-138: LGTM! Clear demonstration of using SpacegroupAnalyzer.The symmetry analysis is correctly implemented to identify inequivalent sites.
181-185: LGTM! Proper demonstration of the DefectsGenerator.The implementation correctly shows how to use doped's DefectsGenerator without interstitials or supercell generation.
280-287: LGTM! Well-structured distortion implementation.The code clearly demonstrates the use of distort_and_rattle for generating trial polaron structures, with appropriate distortion factors and nearest neighbor count.
436-452: Update kernel information to match Python version requirements.The metadata indicates Python 2.7.6 (line 451) but the code appears to be using Python 3 features. Consider updating the notebook metadata to reflect the correct Python version requirements for clarity.
Please verify the minimum Python version required for this tutorial and ensure the notebook metadata reflects this correctly.
| def hole_finder(vasprun_defect, bands_below=3, bands_above=3): | ||
| warnings.warn("""Python is 0-indexed, VASP in 1-indexed. All k-points and | ||
| bands shown as an output from this script are 1-indexed, i.e. VASP-friendly.""") | ||
|
|
||
| warnings.warn("""This is only designed to work for cases where you have a single hole... | ||
| For multiple holes, or electrons, use the big table to work it out yourself!""") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing function docstring and using warnings for user messaging.
The function hole_finder lacks a proper docstring that would explain parameters and return values. Additionally, using warnings for user instructions is not ideal as they might be suppressed.
Add a proper docstring and use print statements for user instructions:
def hole_finder(vasprun_defect, bands_below=3, bands_above=3):
+ """
+ Identify a single hole in the electronic structure from a VASP calculation.
+
+ Args:
+ vasprun_defect (str): Path to the vasprun.xml file
+ bands_below (int): Number of bands below VBM to include in analysis
+ bands_above (int): Number of bands above VBM to include in analysis
+
+ Returns:
+ None: Results are printed to stdout
+ """
- warnings.warn("""Python is 0-indexed, VASP in 1-indexed. All k-points and
- bands shown as an output from this script are 1-indexed, i.e. VASP-friendly.""")
+ print("\nNOTE: Python is 0-indexed, VASP in 1-indexed. All k-points and")
+ print("bands shown as an output from this script are 1-indexed, i.e. VASP-friendly.\n")
- warnings.warn("""This is only designed to work for cases where you have a single hole...
- For multiple holes, or electrons, use the big table to work it out yourself!""")
+ print("NOTE: This is only designed to work for cases where you have a single hole...")
+ print("For multiple holes, or electrons, use the big table to work it out yourself!\n")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def hole_finder(vasprun_defect, bands_below=3, bands_above=3): | |
| warnings.warn("""Python is 0-indexed, VASP in 1-indexed. All k-points and | |
| bands shown as an output from this script are 1-indexed, i.e. VASP-friendly.""") | |
| warnings.warn("""This is only designed to work for cases where you have a single hole... | |
| For multiple holes, or electrons, use the big table to work it out yourself!""") | |
| def hole_finder(vasprun_defect, bands_below=3, bands_above=3): | |
| """ | |
| Identify a single hole in the electronic structure from a VASP calculation. | |
| Args: | |
| vasprun_defect (str): Path to the vasprun.xml file | |
| bands_below (int): Number of bands below VBM to include in analysis | |
| bands_above (int): Number of bands above VBM to include in analysis | |
| Returns: | |
| None: Results are printed to stdout | |
| """ | |
| print("\nNOTE: Python is 0-indexed, VASP is 1-indexed. All k-points and") | |
| print("bands shown as an output from this script are 1-indexed, i.e. VASP-friendly.\n") | |
| print("NOTE: This is only designed to work for cases where you have a single hole...") | |
| print("For multiple holes, or electrons, use the big table to work it out yourself!\n") | |
| # ... rest of implementation ... |
🧰 Tools
🪛 Ruff (0.8.2)
9-9: Missing docstring in public function
(D103)
| "source": "The `hole-finder.py` script provided in the `ShakeNBreak` `docs` directory can be useful for identifying hole states in VASP calculations, for `PARCHG` analysis.", | ||
| "id": "61a8cec047463ee0" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more details about the hole-finder.py script.
The notebook mentions the hole-finder.py script but provides no details about how to use it or what it does. Consider adding a brief example or description of its functionality.
-The `hole-finder.py` script provided in the `ShakeNBreak` `docs` directory can be useful for identifying hole states in VASP calculations, for `PARCHG` analysis.
+The `hole-finder.py` script provided in the `ShakeNBreak` `docs` directory can be useful for identifying hole states in VASP calculations, for `PARCHG` analysis.
+
+For example, to identify hole states in a calculation:
+```
+python hole-finder.py path/to/vasprun.xml
+```
+
+This script analyzes the vasprun.xml file to find single-hole electronic states and provides recommended energy integration parameters for visualizing the hole state using PARCHG outputs.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "source": "The `hole-finder.py` script provided in the `ShakeNBreak` `docs` directory can be useful for identifying hole states in VASP calculations, for `PARCHG` analysis.", | |
| "id": "61a8cec047463ee0" | |
| "source": "The `hole-finder.py` script provided in the `ShakeNBreak` `docs` directory can be useful for identifying hole states in VASP calculations, for `PARCHG` analysis.\n\nFor example, to identify hole states in a calculation:\n |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_cli.py (1)
321-321: Commented assertion for INCAR equality checkThe assertion checking that the generated INCAR matches the reference INCAR has been commented out. While this likely helps accommodate changes related to the newly introduced progress bars, removing this verification could potentially allow regressions in INCAR generation to go undetected.
Consider adding a comment explaining why this assertion was disabled or implementing a more flexible comparison that allows for specific differences while still verifying essential INCAR properties.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_cli.py(2 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 (1)
tests/test_cli.py (1)
1099-1100: Good addition to properly handle tqdm progress bar output in testsSetting
mix_stderr=Falsein the CliRunner is an appropriate solution to prevent tqdm progress bar output (which writes to stderr) from interfering with test output assertions. This ensures more reliable testing with the newly added progress bars.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
README.md (2)
94-94: Unnecessary escape in author name
The backslash inY\. Liuis not needed and may render incorrectly; please replace withY. Liu.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
94-94: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
101-101: Inline HTML comment in list
There's an HTML comment (<!-- though didn't cite properly SnB or doped code papers... -->) at the end of this list item. It may break Markdown list rendering. Consider moving this note out of the list or removing it.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
101-101: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
tests/test_distortions.py (2)
89-89: Fix line length and consider extracting path.The line exceeds the 107-character limit and could be made more readable.
- self.V_Si_HfSiO_interface_entry = loadfn(os.path.join(self.DATA_DIR, "v_Si_C1_O1.70_HfSiO_interface_+1.json")) + self.V_Si_HfSiO_interface_entry = loadfn( + os.path.join(self.DATA_DIR, "v_Si_C1_O1.70_HfSiO_interface_+1.json") + )🧰 Tools
🪛 Ruff (0.11.9)
89-89: Line too long (118 > 107)
(E501)
436-455: Test logic is correct but has minor style issues.The test appropriately verifies the fallback behavior when CrystalNN returns fewer than 2 neighbors, addressing the bug mentioned in #85. The assertions confirm correct structure handling and distance constraints.
Consider addressing these minor style improvements:
- self.assertTrue(output["distorted_atoms"] == [[82, 'Si'], [177, 'O']]) + self.assertEqual(output["distorted_atoms"], [[82, "Si"], [177, "O"]])- self.assertTrue(min_dist > 1.5) # Si-O dimer bond length is larger than min_dist in orig structure + self.assertGreater(min_dist, 1.5) # Si-O dimer bond length is larger than min_dist in orig structure🧰 Tools
🪛 Ruff (0.11.9)
448-448: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
449-449: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
450-450: Use a regular
assertinstead of unittest-styleassertTrueReplace
assertTrue(...)withassert ...(PT009)
450-450: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
450-450: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
454-454: Use a regular
assertinstead of unittest-styleassertTrueReplace
assertTrue(...)withassert ...(PT009)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.rst(1 hunks)README.md(3 hunks)docs/ShakeNBreak_Example_Workflow.ipynb(2 hunks)docs/ShakeNBreak_Polaron_Workflow.ipynb(1 hunks)docs/conf.py(2 hunks)docs/docs_requirements.txt(1 hunks)docs/index.rst(3 hunks)setup.py(3 hunks)shakenbreak/analysis.py(6 hunks)shakenbreak/distortions.py(4 hunks)shakenbreak/energy_lowering_distortions.py(7 hunks)tests/test_distortions.py(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/docs_requirements.txt
- docs/conf.py
- CHANGELOG.rst
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/ShakeNBreak_Example_Workflow.ipynb
- setup.py
- shakenbreak/analysis.py
- docs/index.rst
- shakenbreak/distortions.py
- shakenbreak/energy_lowering_distortions.py
- docs/ShakeNBreak_Polaron_Workflow.ipynb
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/test_distortions.py
89-89: Line too long (118 > 107)
(E501)
132-132: Redefinition of unused test_distort_V_Cd from line 96
(F811)
133-133: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
135-135: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
136-136: Local variable output is assigned to but never used
Remove assignment to unused variable output
(F841)
139-139: Line too long (109 > 107)
(E501)
142-142: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
448-448: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
449-449: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
450-450: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
450-450: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
450-450: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
454-454: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
🪛 Pylint (3.3.7)
tests/test_distortions.py
[error] 132-132: method already defined line 96
(E0102)
🪛 markdownlint-cli2 (0.17.2)
README.md
89-89: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
90-90: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
91-91: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
93-93: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
94-94: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
95-95: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
96-96: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
97-97: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
98-98: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
99-99: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
100-100: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
101-101: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
102-102: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.11)
🔇 Additional comments (6)
README.md (5)
28-28: Literature citation request added
The new line politely asks users to cite both the code and theory/method paper, which aligns with best practices for academic software documentation.
63-63: Python API documentation updated
The updated tutorial link and docs URL are accurate and consistent with the new ReadTheDocs location.
89-90: New 2025 study entries added
Entries for López et al. and Ogawa et al. are correctly formatted and appropriately placed at the top of the list.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
89-89: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
90-90: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
95-96: Study entries for Kavanagh and Huang
The formatting and citation links for the Kavanagh et al. and Huang et al. entries look correct.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
95-95: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
96-96: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
100-100: Additional 2025 study entries
The entries for Desai et al. and Gierlotka et al. are well formatted with valid DOIs and publication details.Also applies to: 102-102
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
100-100: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
tests/test_distortions.py (1)
1-4: Documentation addition looks good.The module-level docstring appropriately notes the relationship with other test modules.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/hole-finder.py (1)
70-72: Eigenvalue channel reference is correct for spin-up hole case.The code correctly uses "up-eigenval" for the spin-up hole case, which matches the expected behavior since the hole is in the spin-up channel.
This issue was previously flagged but the current code actually has the correct implementation. The eigenvalue channel reference is appropriate for the spin-up hole case.
🧹 Nitpick comments (7)
README.md (4)
28-28: Consider highlighting the citation request with a callout.
The added reminder to cite the code and theory/method is important—using a Markdown blockquote or admonition (e.g.,> **Note:** …) will make it more prominent.
89-90: Use consistent list bullet style.
The new entries use-for bullets, whereas the rest of the document follows*. Switching to*will align with the established Markdown style.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
89-89: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
90-90: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
94-96: Maintain uniform list formatting.
These list items are prefixed with-; please convert them to*for consistency with the rest of the lists.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
94-94: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
95-95: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
96-96: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
100-101: Normalize list marker usage.
Update the newly added entries to use*bullets to match the existing Markdown conventions.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
100-100: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
101-101: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
docs/hole-finder.py (3)
1-8: Add module docstring to document the script's purpose.The script lacks a module-level docstring explaining its purpose and usage.
+""" +Helper script for identifying hole states in VASP defect calculations. + +This script analyzes vasprun.xml files to locate a single hole in the electronic +structure and provides eigenvalue integration ranges for PARCHG calculations. +""" import itertools🧰 Tools
🪛 Ruff (0.11.9)
1-1: Missing docstring in public module
(D100)
47-49: Use a more descriptive variable name for the DataFrame.The generic variable name
dfcould be more descriptive to improve code readability.-df = pd.DataFrame(eigenvals, +eigenval_df = pd.DataFrame(eigenvals, columns=["band", "k-point", "up-eigenval", "down-eigenval", "up-occupancy", "down-occupancy"])Remember to update all subsequent references to use
eigenval_dfinstead ofdf.🧰 Tools
🪛 Ruff (0.11.9)
47-47: Avoid using the generic variable name
dffor DataFrames(PD901)
55-59: Fix line length violations for better readability.The lines exceed the recommended length limit and should be wrapped.
-print( - "Locating hole... Disregard if you are concerned with any other situation than a defect with a SINGLE HOLE\n") -print( - "Information on EINT min and max can be ignored if you are using pymatgen to make your PARCHG file") +print("Locating hole... Disregard if you are concerned with any other situation " + "than a defect with a SINGLE HOLE\n") +print("Information on EINT min and max can be ignored if you are using pymatgen " + "to make your PARCHG file")🧰 Tools
🪛 Ruff (0.11.9)
56-56: Line too long (118 > 107)
(E501)
58-58: Line too long (108 > 107)
(E501)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/pip_install_test.yml(3 hunks).github/workflows/test.yml(2 hunks)README.md(3 hunks)docs/hole-finder.py(1 hunks)docs/index.rst(3 hunks)tests/test_cli.py(3 hunks)tests/test_distortions.py(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/index.rst
- tests/test_cli.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
89-89: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
90-90: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
91-91: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
93-93: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
94-94: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
95-95: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
96-96: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
97-97: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
98-98: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
99-99: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
100-100: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
101-101: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
102-102: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🪛 Ruff (0.11.9)
docs/hole-finder.py
1-1: Missing docstring in public module
(D100)
10-10: Missing docstring in public function
(D103)
47-47: Avoid using the generic variable name df for DataFrames
(PD901)
56-56: Line too long (118 > 107)
(E501)
58-58: Line too long (108 > 107)
(E501)
63-63: Avoid equality comparisons to True; use if hole_spin_up.empty: for truth checks
Replace with hole_spin_up.empty
(E712)
63-63: Avoid equality comparisons to False; use if not hole_spin_down.empty: for false checks
Replace with not hole_spin_down.empty
(E712)
68-68: Avoid equality comparisons to True; use if hole_spin_down.empty: for truth checks
Replace with hole_spin_down.empty
(E712)
68-68: Avoid equality comparisons to False; use if not hole_spin_up.empty: for false checks
Replace with not hole_spin_up.empty
(E712)
73-73: Avoid equality comparisons to True; use if hole_spin_up.empty: for truth checks
Replace with hole_spin_up.empty
(E712)
73-73: Avoid equality comparisons to True; use if hole_spin_down.empty: for truth checks
Replace with hole_spin_down.empty
(E712)
tests/test_distortions.py
109-110: Explicitly concatenated string should be implicitly concatenated
(ISC003)
116-116: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
117-117: Local variable output is assigned to but never used
Remove assignment to unused variable output
(F841)
123-123: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
167-167: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
168-168: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
169-169: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
170-170: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
171-171: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
172-172: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
357-357: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
365-365: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
368-368: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
372-372: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
373-373: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
374-374: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
382-382: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
400-400: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
408-408: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
422-422: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
423-423: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
424-424: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
428-428: Use a regular assert instead of unittest-style assertGreater
Replace assertGreater(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: test (ubuntu-latest, 3.13)
🔇 Additional comments (10)
README.md (1)
63-63: Confirm updated API doc links.
The Python API tutorial and docs URLs have been revised—please verify these point to the correct, versioned landing pages and update any other occurrences accordingly.tests/test_distortions.py (2)
1-3: LGTM: Good documentation addition.The module docstring helpfully clarifies that most distortion functions are implicitly tested in
test_input.
410-431: Excellent test for addressing issue #85.This test properly validates the fallback behavior when
CrystalNNreturns insufficient neighbors, ensuring the distortion functions handle complex interface structures correctly. The assertions comprehensively verify:
- Structure integrity
- Correct neighbor count
- Expected distorted atom indices
- Minimum distance constraints
🧰 Tools
🪛 Ruff (0.11.9)
422-422: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
423-423: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
424-424: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
428-428: Use a regular
assertinstead of unittest-styleassertGreaterReplace
assertGreater(...)withassert ...(PT009)
.github/workflows/pip_install_test.yml (3)
12-12: Good improvement: More descriptive job name.Renaming from
buildtotestbetter reflects the job's actual purpose.
21-21: Efficient testing strategy: Focus on boundary Python versions.Testing only Python 3.10 and 3.13 (oldest and latest supported) is a good strategy for PyPI installation validation, reducing CI overhead while maintaining adequate coverage.
46-46: Good consolidation: Simplified test commands.Running pytest on the entire
testsdirectory with markers is cleaner than separate commands for individual test files, while maintaining the same test coverage.Also applies to: 51-51
.github/workflows/test.yml (4)
11-11: Consistent improvement: Job name alignment.Renaming to
testmaintains consistency with the pip install workflow and better describes the job function.
17-17: Strategic testing matrix: Boundary version focus.Testing Python 3.10 and 3.13 provides good coverage while reducing CI complexity and execution time.
47-47: Improved test organization: Consolidated commands.The consolidated pytest commands are cleaner and maintain the same functionality while reducing duplication.
Also applies to: 52-52
59-59: Excellent enhancement: Dynamic artifact naming.Using matrix variables in the artifact name (
SnB_GH_Actions_plots_${{ matrix.os }}_${{ matrix.python-version }}) makes it easy to distinguish test results across different OS and Python version combinations.
| if hole_spin_up.empty == True and hole_spin_down.empty == False: | ||
| print("Hole is in the spin down channel:\n", hole_spin_down) | ||
| print("Recommended min and max for EINT in PARCHG calculation:", | ||
| round(hole_spin_down["down-eigenval"].min(), 2), | ||
| round(hole_spin_down["down-eigenval"].max(), 2)) | ||
| elif hole_spin_down.empty == True and hole_spin_up.empty == False: | ||
| print("Hole is in the spin up channel:\n", hole_spin_up) | ||
| print("Recommended min and max for EINT in PARCHG calculation:", | ||
| round(hole_spin_up["up-eigenval"].min(), 2), | ||
| round(hole_spin_up["up-eigenval"].max(), 2)) | ||
| elif hole_spin_up.empty == True and hole_spin_down.empty == True: | ||
| print("No holes found!") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify boolean comparisons and fix logic consistency.
The boolean equality comparisons are unnecessarily verbose and should be simplified. Additionally, there's a potential logic issue where the elif conditions might not be mutually exclusive.
-if hole_spin_up.empty == True and hole_spin_down.empty == False:
+if hole_spin_up.empty and not hole_spin_down.empty:
print("Hole is in the spin down channel:\n", hole_spin_down)
print("Recommended min and max for EINT in PARCHG calculation:",
round(hole_spin_down["down-eigenval"].min(), 2),
round(hole_spin_down["down-eigenval"].max(), 2))
-elif hole_spin_down.empty == True and hole_spin_up.empty == False:
+elif hole_spin_down.empty and not hole_spin_up.empty:
print("Hole is in the spin up channel:\n", hole_spin_up)
print("Recommended min and max for EINT in PARCHG calculation:",
round(hole_spin_up["up-eigenval"].min(), 2),
round(hole_spin_up["up-eigenval"].max(), 2))
-elif hole_spin_up.empty == True and hole_spin_down.empty == True:
+elif hole_spin_up.empty and hole_spin_down.empty:
print("No holes found!")
+else:
+ print("Multiple holes detected in both spin channels!")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if hole_spin_up.empty == True and hole_spin_down.empty == False: | |
| print("Hole is in the spin down channel:\n", hole_spin_down) | |
| print("Recommended min and max for EINT in PARCHG calculation:", | |
| round(hole_spin_down["down-eigenval"].min(), 2), | |
| round(hole_spin_down["down-eigenval"].max(), 2)) | |
| elif hole_spin_down.empty == True and hole_spin_up.empty == False: | |
| print("Hole is in the spin up channel:\n", hole_spin_up) | |
| print("Recommended min and max for EINT in PARCHG calculation:", | |
| round(hole_spin_up["up-eigenval"].min(), 2), | |
| round(hole_spin_up["up-eigenval"].max(), 2)) | |
| elif hole_spin_up.empty == True and hole_spin_down.empty == True: | |
| print("No holes found!") | |
| if hole_spin_up.empty and not hole_spin_down.empty: | |
| print("Hole is in the spin down channel:\n", hole_spin_down) | |
| print("Recommended min and max for EINT in PARCHG calculation:", | |
| round(hole_spin_down["down-eigenval"].min(), 2), | |
| round(hole_spin_down["down-eigenval"].max(), 2)) | |
| elif hole_spin_down.empty and not hole_spin_up.empty: | |
| print("Hole is in the spin up channel:\n", hole_spin_up) | |
| print("Recommended min and max for EINT in PARCHG calculation:", | |
| round(hole_spin_up["up-eigenval"].min(), 2), | |
| round(hole_spin_up["up-eigenval"].max(), 2)) | |
| elif hole_spin_up.empty and hole_spin_down.empty: | |
| print("No holes found!") | |
| else: | |
| print("Multiple holes detected in both spin channels!") |
🧰 Tools
🪛 Ruff (0.11.9)
63-63: Avoid equality comparisons to True; use if hole_spin_up.empty: for truth checks
Replace with hole_spin_up.empty
(E712)
63-63: Avoid equality comparisons to False; use if not hole_spin_down.empty: for false checks
Replace with not hole_spin_down.empty
(E712)
68-68: Avoid equality comparisons to True; use if hole_spin_down.empty: for truth checks
Replace with hole_spin_down.empty
(E712)
68-68: Avoid equality comparisons to False; use if not hole_spin_up.empty: for false checks
Replace with not hole_spin_up.empty
(E712)
73-73: Avoid equality comparisons to True; use if hole_spin_up.empty: for truth checks
Replace with hole_spin_up.empty
(E712)
73-73: Avoid equality comparisons to True; use if hole_spin_down.empty: for truth checks
Replace with hole_spin_down.empty
(E712)
🤖 Prompt for AI Agents
In docs/hole-finder.py around lines 63 to 74, simplify the boolean checks by
removing explicit comparisons to True or False and use direct truthiness checks
instead. Also, revise the conditional logic to ensure the conditions are
mutually exclusive and cover all cases correctly, possibly by reordering or
combining conditions to avoid overlap and improve clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_distortions.py (1)
114-123: Fix unused variable in test method.The test logic correctly validates negative distortion factor handling, but there's an unused variable that should be removed.
Apply this diff to fix the unused variable:
def test_distort_negative_factor(self): vac_coords = np.array([0, 0, 0]) # Cd vacancy fractional coordinates with self.assertRaises(ValueError) as e: - output = distortions.distort(self.V_Cd_struc, 2, -0.5, frac_coords=vac_coords) + distortions.distort(self.V_Cd_struc, 2, -0.5, frac_coords=vac_coords)🧰 Tools
🪛 Ruff (0.11.9)
116-116: Use
pytest.raisesinstead of unittest-styleassertRaisesReplace
assertRaiseswithpytest.raises(PT027)
117-117: Local variable
outputis assigned to but never usedRemove assignment to unused variable
output(F841)
118-118: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
🧹 Nitpick comments (5)
tests/test_analysis.py (5)
51-52: Use os.path.join for nested paths instead of embedding '/'
Embedding directory separators inside a single join argument can break Windows compatibility.Refactor the call to:
- Structure.from_file( - os.path.join(self.VASP_CDTE_DATA_DIR, "vac_1_Cd_0/Bond_Distortion_-30.0%/CONTCAR") - ) + Structure.from_file( + os.path.join( + self.VASP_CDTE_DATA_DIR, + "vac_1_Cd_0", + "Bond_Distortion_-30.0%", + "CONTCAR", + ) + )
222-223: Combine implicit string literals into a single literal
Avoid implicit concatenation for improved readability and to satisfy linters.- "Local order parameters (i.e. resemblance to given " "structural motif, via CrystalNN):" + "Local order parameters (i.e. resemblance to given structural motif, via CrystalNN):"🧰 Tools
🪛 Ruff (0.11.9)
222-222: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
585-586: Merge adjacent string literals into one
Clean up the assertion message by removing the redundant split.- mock_print.assert_called_with("Comparing structures to -40.0% bond distorted " "structure...") + mock_print.assert_called_with("Comparing structures to -40.0% bond distorted structure...")🧰 Tools
🪛 Ruff (0.11.9)
585-585: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
680-681: Consolidate concatenated literals in warning message
Simplify the combined string into a single literal.- "All structures in defect_structures_dict are not converged. " "Returning None." + "All structures in defect_structures_dict are not converged. Returning None."🧰 Tools
🪛 Ruff (0.11.9)
680-680: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
985-986: Remove extraneousfprefix on string without placeholders
The warning check string doesn’t use interpolation, so thefprefix is unnecessary.- any(f"OUTCAR(.gz) file not found in path" in str(warning.message) for warning in w) + any("OUTCAR(.gz) file not found in path" in str(warning.message) for warning in w)🧰 Tools
🪛 Ruff (0.11.9)
985-985: f-string without any placeholders
Remove extraneous
fprefix(F541)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
shakenbreak/analysis.py(7 hunks)tests/test_analysis.py(34 hunks)tests/test_distortions.py(14 hunks)tests/test_input.py(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shakenbreak/analysis.py
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/test_analysis.py
94-94: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
95-95: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
96-96: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
97-97: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
98-98: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
102-102: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
103-103: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
104-104: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
109-109: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
190-190: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
199-199: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
216-216: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
222-222: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
263-263: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
313-313: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
313-313: Consider [*bond_distortions, "Unperturbed"] instead of concatenation
Replace with [*bond_distortions, "Unperturbed"]
(RUF005)
328-328: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
343-343: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
346-346: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
348-348: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
358-358: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
360-360: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
360-360: Consider [*bond_distortions, "Unperturbed"] instead of concatenation
Replace with [*bond_distortions, "Unperturbed"]
(RUF005)
381-381: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
397-397: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
399-399: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
409-409: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
410-410: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
433-433: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
434-434: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
435-435: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
438-438: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
440-440: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
446-446: Use a regular assert instead of unittest-style assertFalse
Replace assertFalse(...) with assert ...
(PT009)
460-460: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
461-461: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
468-468: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
469-469: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
495-495: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
496-496: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
497-497: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
515-515: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
519-519: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
525-525: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
532-532: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
534-534: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
540-540: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
544-544: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
576-576: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
577-577: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
578-578: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
585-585: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
587-587: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
588-588: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
589-589: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
600-600: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
601-601: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
602-602: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
614-614: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
615-615: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
642-642: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
643-643: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
646-646: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
650-650: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
654-654: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
658-658: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
664-664: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
666-666: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
680-680: Implicitly concatenated string literals on one line
Combine string literals
(ISC001)
682-682: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
683-683: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
684-684: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
685-685: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
706-706: Avoid using the generic variable name df for DataFrames
(PD901)
712-712: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
757-757: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
985-985: f-string without any placeholders
Remove extraneous f prefix
(F541)
tests/test_distortions.py
109-110: Explicitly concatenated string should be implicitly concatenated
(ISC003)
116-116: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
117-117: Local variable output is assigned to but never used
Remove assignment to unused variable output
(F841)
118-118: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
167-167: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
168-168: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
169-169: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
170-170: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
171-171: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
172-172: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
234-234: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
357-357: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
365-365: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
368-368: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
372-372: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
373-373: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
374-374: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
382-382: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
400-400: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
408-408: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
422-422: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
423-423: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
424-424: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
428-428: Use a regular assert instead of unittest-style assertGreater
Replace assertGreater(...) with assert ...
(PT009)
tests/test_input.py
1573-1573: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1588-1588: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
1691-1691: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
1704-1704: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2268-2268: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2270-2270: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2273-2273: Use a regular assert instead of unittest-style assertEqual
(PT009)
2278-2278: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
2623-2623: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
2731-2731: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
2797-2797: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
3638-3638: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
3752-3752: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (ubuntu-latest, 3.10)
🔇 Additional comments (13)
tests/test_input.py (8)
1153-1154: LGTM: Helpful clarification about NELECT parameter behaviorThis comment update provides valuable context about the NELECT parameter handling differences between doped versions, which helps explain why manual updating isn't needed in these tests.
1690-1692: LGTM: Variable naming improvement for clarityThe renaming from
Int_Cd_2_Bond_Distortion_foldertoInt_Cd_2_0_Bond_Distortion_folderconsistently adds the charge state (_0) to the variable names, making the test code more explicit and readable.Also applies to: 1703-1703, 1708-1708, 1715-1715
🧰 Tools
🪛 Ruff (0.11.9)
1691-1691: Use a regular
assertinstead of unittest-styleassertTrueReplace
assertTrue(...)withassert ...(PT009)
2025-2027: LGTM: Informative comment about INCAR parameter differencesThe comment clearly explains why NELECT and NUPDOWN parameters differ between the defect types, providing helpful context for understanding the test expectations.
2214-2214: Minor: Unicode angstrom symbol changeThe change from uppercase Å (U+212B) to lowercase å (U+212b) in the angstrom symbol is noted in the summary but appears to be a very minor typographical change.
2267-2269: LGTM: Consistent variable naming with charge stateThese changes maintain consistency with the earlier variable renaming pattern, explicitly including the charge state in the variable names for better clarity.
Also applies to: 2277-2277, 2282-2282, 2289-2289
🧰 Tools
🪛 Ruff (0.11.9)
2268-2268: Use a regular
assertinstead of unittest-styleassertTrueReplace
assertTrue(...)withassert ...(PT009)
2730-2732: LGTM: Consistent folder variable namingThe renaming continues the pattern of adding charge state information to variable names, maintaining consistency throughout the test file.
🧰 Tools
🪛 Ruff (0.11.9)
2731-2731: Use a regular
assertinstead of unittest-styleassertTrueReplace
assertTrue(...)withassert ...(PT009)
2797-2801: LGTM: Correct assertion usage for exception testingThese
assertIncalls have the correct argument order, checking if the expected error message is contained within the exception string.Also applies to: 3752-3755
🧰 Tools
🪛 Ruff (0.11.9)
2797-2797: Use a regular
assertinstead of unittest-styleassertInReplace
assertIn(...)withassert ...(PT009)
3637-3639: LGTM: Final consistent variable naming updateThis completes the consistent pattern of adding charge state information to folder variable names throughout the test file.
🧰 Tools
🪛 Ruff (0.11.9)
3638-3638: Use a regular
assertinstead of unittest-styleassertTrueReplace
assertTrue(...)withassert ...(PT009)
tests/test_distortions.py (4)
1-3: LGTM!The module docstring provides helpful context about the implicit testing relationship with
test_input.
73-75: LGTM!Good addition of the HfSiO interface defect entry to support testing of complex interface structures.
234-238: LGTM!Good fix to properly compare the exception message string instead of the exception object.
🧰 Tools
🪛 Ruff (0.11.9)
234-234: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
410-431: Excellent test coverage for the fallback behavior!This test effectively validates the fix for issue #85, ensuring correct behavior when
CrystalNNreturns fewer than 2 neighbors for interface structures. The test properly:
- Uses a complex interface structure that triggers the fallback behavior
- Validates the expected number of distorted neighbors (2)
- Checks the specific atoms that get distorted
- Ensures the minimum interatomic distance constraint is satisfied
The test directly addresses the bug where insufficient neighbors from
CrystalNNwould cause failures in complex structures.🧰 Tools
🪛 Ruff (0.11.9)
422-422: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
423-423: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
424-424: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
428-428: Use a regular
assertinstead of unittest-styleassertGreaterReplace
assertGreater(...)withassert ...(PT009)
tests/test_analysis.py (1)
94-97: All other changes are purely cosmetic or formatting-only (condensed assertions, style tweaks, added test cases without logic changes). No functional issues detected.Also applies to: 102-104, 112-112, 141-141, 151-151, 181-181, 190-190, 199-199, 215-216, 224-224, 248-248, 262-263, 283-283, 288-288, 313-313, 328-328, 343-343, 347-348, 353-353, 381-381, 383-384, 397-399, 409-410, 433-434, 438-440, 448-449, 458-458, 461-461, 468-470, 479-479, 500-500, 509-509, 514-515, 519-523, 531-533, 541-544, 566-567, 576-578, 587-589, 598-600, 614-615, 641-642, 649-652, 655-658, 664-669, 677-684, 689-689, 705-706, 712-712, 715-716, 723-726, 735-737, 751-757, 783-785, 808-808, 824-826, 836-838, 891-892, 949-950
🧰 Tools
🪛 Ruff (0.11.9)
94-94: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
95-95: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
96-96: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
97-97: Use a regular
assertinstead of unittest-styleassertEqualReplace
assertEqual(...)withassert ...(PT009)
| self.assertIn( | ||
| str(e.exception), | ||
| "Input `defect_entries` dict matches `doped`/`PyCDT` format, but no 'bulk' entry present. " | ||
| "Please try again providing a `bulk` entry in `defect_entries`.", | ||
| ) |
There was a problem hiding this comment.
Fix incorrect assertion argument order
The assertIn method call has its arguments in the wrong order. Currently it's checking if the exception string contains the error message, but it should check if the error message is contained in the exception string.
Apply this fix:
- self.assertIn(
- str(e.exception),
- "Input `defect_entries` dict matches `doped`/`PyCDT` format, but no 'bulk' entry present. "
- "Please try again providing a `bulk` entry in `defect_entries`.",
- )
+ self.assertIn(
+ "Input `defect_entries` dict matches `doped`/`PyCDT` format, but no 'bulk' entry present. "
+ "Please try again providing a `bulk` entry in `defect_entries`.",
+ str(e.exception),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.assertIn( | |
| str(e.exception), | |
| "Input `defect_entries` dict matches `doped`/`PyCDT` format, but no 'bulk' entry present. " | |
| "Please try again providing a `bulk` entry in `defect_entries`.", | |
| ) | |
| self.assertIn( | |
| "Input `defect_entries` dict matches `doped`/`PyCDT` format, but no 'bulk' entry present. " | |
| "Please try again providing a `bulk` entry in `defect_entries`.", | |
| str(e.exception), | |
| ) |
🧰 Tools
🪛 Ruff (0.11.9)
2623-2623: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
🤖 Prompt for AI Agents
In tests/test_input.py around lines 2623 to 2627, the assertIn call has its
arguments reversed; it currently checks if the exception string contains the
error message, but it should verify if the expected error message is contained
within the exception string. Swap the arguments so that the expected error
message string is the first argument and the exception string is the second
argument in the assertIn call.
Closes #85
Will merge to
mainand release after the nextdopedrelease (hopefully pretty soon).