Fix ruff FURB192
#3785
Fix ruff FURB192
#3785
Conversation
WalkthroughThe updates involve modernizing Python syntax across multiple files, enhancing code readability and efficiency. Key changes include upgrading dependency versions in pre-commit hooks, streamlining imports, refining variable names for clarity, and adopting f-strings for string formatting. Improvements also focus on optimizing logic in parsing and data handling functions, particularly within various modules related to materials science and computational chemistry software. Changes
This table summarizes the changes made across various files, focusing on enhancements in code quality and maintenance. Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (55)
Files not processed due to max files limit (16)
Files skipped from review due to trivial changes (7)
Additional comments not posted (98)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration 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: 15
Out of diff range and nitpick comments (2)
pymatgen/io/abinit/inputs.py (1)
872-872: Consider adding a brief comment explaining the purpose of thepsp_dictdictionary.This would improve the readability and maintainability of the code by making the intentions behind the code clearer to other developers.
pymatgen/io/vasp/outputs.py (1)
3572-3572: Clarify the purpose of thefft_meshmethod in the documentation.The documentation for the
fft_meshmethod could be improved to better explain its purpose and usage. Consider adding more details about how it interacts with other components of the system and any expected inputs or outputs.
| @@ -205,7 +206,7 @@ def __add__(self, other: object) -> Composition: | |||
| if not isinstance(other, (Composition, dict)): | |||
| return NotImplemented | |||
|
|
|||
| new_el_map: dict[SpeciesLike, float] = collections.defaultdict(float) | |||
| new_el_map: dict[SpeciesLike, float] = defaultdict(float) | |||
There was a problem hiding this comment.
Replace defaultdict with a direct import to enhance code clarity and consistency.
- from collections import defaultdict
+ from collections import defaultdictCommittable 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.
| new_el_map: dict[SpeciesLike, float] = defaultdict(float) | |
| new_el_map: dict[SpeciesLike, float] = defaultdict(float) |
| @@ -534,7 +535,7 @@ def _parse_formula(self, formula: str, strict: bool = True) -> dict[str, float]: | |||
| formula = formula.replace("]", ")") | |||
|
|
|||
| def get_sym_dict(form: str, factor: float) -> dict[str, float]: | |||
| sym_dict: dict[str, float] = collections.defaultdict(float) | |||
| sym_dict: dict[str, float] = defaultdict(float) | |||
There was a problem hiding this comment.
Consider using a direct import for defaultdict to align with changes made in other parts of the codebase.
- from collections import defaultdict
+ from collections import defaultdictCommittable 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.
| sym_dict: dict[str, float] = defaultdict(float) | |
| sym_dict: dict[str, float] = defaultdict(float) |
| @@ -638,7 +639,7 @@ def get_el_amt_dict(self) -> dict[str, float]: | |||
| dict[str, float]: element symbol and (unreduced) amount. E.g. | |||
| {"Fe": 4.0, "O": 6.0}. | |||
| """ | |||
| dct: dict[str, float] = collections.defaultdict(float) | |||
| dct: dict[str, float] = defaultdict(float) | |||
There was a problem hiding this comment.
Maintain consistency in the import of defaultdict to improve code readability.
- from collections import defaultdict
+ from collections import defaultdictCommittable 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.
| dct: dict[str, float] = defaultdict(float) | |
| dct: dict[str, float] = defaultdict(float) |
| @@ -651,7 +652,7 @@ def as_dict(self) -> dict[str, float]: | |||
| dict[str, float]: element symbol and (unreduced) amount. E.g. | |||
| {"Fe": 4.0, "O": 6.0} or {"Fe3+": 4.0, "O2-": 6.0} | |||
| """ | |||
| dct: dict[str, float] = collections.defaultdict(float) | |||
| dct: dict[str, float] = defaultdict(float) | |||
There was a problem hiding this comment.
Use a direct import for defaultdict to align with the refactoring strategy applied in other parts of the project.
- from collections import defaultdict
+ from collections import defaultdictCommittable 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.
| dct: dict[str, float] = defaultdict(float) | |
| dct: dict[str, float] = defaultdict(float) |
| @@ -873,7 +874,7 @@ def remove_charges(self) -> Composition: | |||
| Composition object without charge decoration, for example | |||
| {"Fe3+": 2.0, "O2-":3.0} becomes {"Fe": 2.0, "O":3.0} | |||
| """ | |||
| dct: dict[Element, float] = collections.defaultdict(float) | |||
| dct: dict[Element, float] = defaultdict(float) | |||
There was a problem hiding this comment.
Adopt a direct import of defaultdict to ensure consistency and clarity in the codebase.
- from collections import defaultdict
+ from collections import defaultdictCommittable 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.
| dct: dict[Element, float] = defaultdict(float) | |
| dct: dict[Element, float] = defaultdict(float) |
| match = coord_patt.search(line) | ||
| if match: | ||
| sites.append([match.groups()[0], list(map(float, match.groups()[2:]))]) | ||
| else: | ||
| m = final_energy_patt.search(line) | ||
| if m: | ||
| self.final_energy = float(m.group(1)) | ||
| match = final_energy_patt.search(line) | ||
| if match: | ||
| self.final_energy = float(match.group(1)) |
There was a problem hiding this comment.
Optimize the structure parsing for 'SinglePoint' run type.
- match = coord_patt.search(line)
- if match:
- sites.append([match.groups()[0], list(map(float, match.groups()[2:]))])
- else:
- match = final_energy_patt.search(line)
- if match:
- self.final_energy = float(match.group(1))
- self.final_structure = self._sites_to_mol(sites)
+ if (match := coord_patt.search(line)):
+ sites.append([match.groups()[0], list(map(float, match.groups()[2:]))])
+ elif (match := final_energy_patt.search(line)):
+ self.final_energy, self.final_structure = float(match.group(1)), self._sites_to_mol(sites)This refactoring uses the walrus operator to simplify the logic, making the code more concise and easier to read.
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.
| match = coord_patt.search(line) | |
| if match: | |
| sites.append([match.groups()[0], list(map(float, match.groups()[2:]))]) | |
| else: | |
| m = final_energy_patt.search(line) | |
| if m: | |
| self.final_energy = float(m.group(1)) | |
| match = final_energy_patt.search(line) | |
| if match: | |
| self.final_energy = float(match.group(1)) | |
| if (match := coord_patt.search(line)): | |
| sites.append([match.groups()[0], list(map(float, match.groups()[2:]))]) | |
| elif (match := final_energy_patt.search(line)): | |
| self.final_energy, self.final_structure = float(match.group(1)), self._sites_to_mol(sites) |
| @@ -270,7 +270,7 @@ def write_lobsterin(self, path="lobsterin", overwritedict=None): | |||
| # checks if entry is True or False | |||
| for key_here in self: | |||
| if key.lower() == key_here.lower(): | |||
| file.write(key + "\n") | |||
| file.write(f"{key}\n") | |||
There was a problem hiding this comment.
Use f-strings consistently for file writing.
- file.write(f"{key}\n")
+ file.write(f"{key_here}\n")This change ensures that the actual key used in the dictionary (which might have different casing) is written to the file, maintaining consistency with the internal data representation.
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.
| file.write(f"{key}\n") | |
| file.write(f"{key_here}\n") |
| for key, setting in settings.items(): | ||
| if key == "MAGMOM": | ||
| mag = [] | ||
| for site in structure: | ||
| if hasattr(site, "magmom"): | ||
| mag.append(site.magmom) | ||
| elif getattr(site.specie, "spin", None) is not None: | ||
| mag.append(site.specie.spin) | ||
| elif str(site.specie) in v: | ||
| if site.specie.symbol == "Co" and v[str(site.specie)] <= 1.0: | ||
| elif str(site.specie) in setting: | ||
| if site.specie.symbol == "Co" and setting[str(site.specie)] <= 1.0: | ||
| warnings.warn( | ||
| "Co without an oxidation state is initialized as low spin by default in Pymatgen. " | ||
| "If this default behavior is not desired, please set the spin on the magmom on the " | ||
| "site directly to ensure correct initialization." | ||
| ) | ||
| mag.append(v.get(str(site.specie))) | ||
| mag.append(setting.get(str(site.specie))) | ||
| else: | ||
| if site.specie.symbol == "Co": | ||
| warnings.warn( | ||
| "Co without an oxidation state is initialized as low spin by default in Pymatgen. " | ||
| "If this default behavior is not desired, please set the spin on the magmom on the " | ||
| "site directly to ensure correct initialization." | ||
| ) | ||
| mag.append(v.get(site.specie.symbol, 0.6)) | ||
| incar[k] = mag | ||
| elif k in ("LDAUU", "LDAUJ", "LDAUL"): | ||
| mag.append(setting.get(site.specie.symbol, 0.6)) | ||
| incar[key] = mag | ||
| elif key in ("LDAUU", "LDAUJ", "LDAUL"): | ||
| if hubbard_u: | ||
| if hasattr(structure[0], k.lower()): | ||
| m = {site.specie.symbol: getattr(site, k.lower()) for site in structure} | ||
| incar[k] = [m[sym] for sym in poscar.site_symbols] | ||
| if hasattr(structure[0], key.lower()): | ||
| m = {site.specie.symbol: getattr(site, key.lower()) for site in structure} | ||
| incar[key] = [m[sym] for sym in poscar.site_symbols] | ||
| # lookup specific LDAU if specified for most_electroneg atom | ||
| elif most_electro_neg in v and isinstance(v[most_electro_neg], dict): | ||
| incar[k] = [v[most_electro_neg].get(sym, 0) for sym in poscar.site_symbols] | ||
| elif most_electro_neg in setting and isinstance(setting[most_electro_neg], dict): | ||
| incar[key] = [setting[most_electro_neg].get(sym, 0) for sym in poscar.site_symbols] | ||
| # else, use fallback LDAU value if it exists | ||
| else: | ||
| incar[k] = [ | ||
| v.get(sym, 0) if isinstance(v.get(sym, 0), (float, int)) else 0 | ||
| incar[key] = [ | ||
| setting.get(sym, 0) if isinstance(setting.get(sym, 0), (float, int)) else 0 | ||
| for sym in poscar.site_symbols | ||
| ] | ||
| elif k.startswith("EDIFF") and k != "EDIFFG": | ||
| if "EDIFF" not in settings and k == "EDIFF_PER_ATOM": | ||
| incar["EDIFF"] = float(v) * len(structure) | ||
| elif key.startswith("EDIFF") and key != "EDIFFG": | ||
| if "EDIFF" not in settings and key == "EDIFF_PER_ATOM": | ||
| incar["EDIFF"] = float(setting) * len(structure) | ||
| else: | ||
| incar["EDIFF"] = float(settings["EDIFF"]) | ||
| elif k == "KSPACING" and v == "auto": | ||
| elif key == "KSPACING" and setting == "auto": | ||
| # default to metal if no prev calc available | ||
| bandgap = 0 if self.bandgap is None else self.bandgap | ||
| incar[k] = auto_kspacing(bandgap, self.bandgap_tol) | ||
| incar[key] = auto_kspacing(bandgap, self.bandgap_tol) | ||
| else: | ||
| incar[k] = v | ||
| incar[key] = setting |
There was a problem hiding this comment.
Refactor the incar property to improve readability and maintainability.
- for key, setting in settings.items():
- if key == "MAGMOM":
- mag = []
- for site in structure:
- if hasattr(site, "magmom"):
- mag.append(site.magmom)
- elif getattr(site.specie, "spin", None) is not None:
- mag.append(site.specie.spin)
- elif str(site.specie) in setting:
- if site.specie.symbol == "Co" and setting[str(site.specie)] <= 1.0:
- warnings.warn(
- "Co without an oxidation state is initialized as low spin by default in Pymatgen. "
- "If this default behavior is not desired, please set the spin on the magmom on the "
- "site directly to ensure correct initialization."
- )
- mag.append(setting.get(str(site.specie)))
- else:
- if site.specie.symbol == "Co":
- warnings.warn(
- "Co without an oxidation state is initialized as low spin by default in Pymatgen. "
- "If this default behavior is not desired, please set the spin on the magmom on the "
- "site directly to ensure correct initialization."
- )
- mag.append(setting.get(site.specie.symbol, 0.6))
- incar[key] = mag
- elif key in ("LDAUU", "LDAUJ", "LDAUL"):
- if hubbard_u:
- if hasattr(structure[0], key.lower()):
- m = {site.specie.symbol: getattr(site, key.lower()) for site in structure}
- incar[key] = [m[sym] for sym in poscar.site_symbols]
- elif most_electro_neg in setting and isinstance(setting[most_electro_neg], dict):
- incar[key] = [setting[most_electro_neg].get(sym, 0) for sym in poscar.site_symbols]
- else:
- incar[key] = [
- setting.get(sym, 0) if isinstance(setting.get(sym, 0), (float, int)) else 0
- for sym in poscar.site_symbols
- ]
- elif key.startswith("EDIFF") and key != "EDIFFG":
- if "EDIFF" not in settings and key == "EDIFF_PER_ATOM":
- incar["EDIFF"] = float(setting) * len(structure)
- else:
- incar["EDIFF"] = float(settings["EDIFF"])
- elif key == "KSPACING" and setting == "auto":
- bandgap = 0 if self.bandgap is None else self.bandgap
- incar[key] = auto_kspacing(bandgap, self.bandgap_tol)
- else:
- incar[key] = setting
+ _apply_incar_updates(incar, settings, structure, poscar, hubbard_u, most_electro_neg)This refactoring suggestion aims to extract the complex logic inside the incar property into a separate function _apply_incar_updates. This will make the incar property cleaner and the logic more reusable and testable.
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.
| for key, setting in settings.items(): | |
| if key == "MAGMOM": | |
| mag = [] | |
| for site in structure: | |
| if hasattr(site, "magmom"): | |
| mag.append(site.magmom) | |
| elif getattr(site.specie, "spin", None) is not None: | |
| mag.append(site.specie.spin) | |
| elif str(site.specie) in v: | |
| if site.specie.symbol == "Co" and v[str(site.specie)] <= 1.0: | |
| elif str(site.specie) in setting: | |
| if site.specie.symbol == "Co" and setting[str(site.specie)] <= 1.0: | |
| warnings.warn( | |
| "Co without an oxidation state is initialized as low spin by default in Pymatgen. " | |
| "If this default behavior is not desired, please set the spin on the magmom on the " | |
| "site directly to ensure correct initialization." | |
| ) | |
| mag.append(v.get(str(site.specie))) | |
| mag.append(setting.get(str(site.specie))) | |
| else: | |
| if site.specie.symbol == "Co": | |
| warnings.warn( | |
| "Co without an oxidation state is initialized as low spin by default in Pymatgen. " | |
| "If this default behavior is not desired, please set the spin on the magmom on the " | |
| "site directly to ensure correct initialization." | |
| ) | |
| mag.append(v.get(site.specie.symbol, 0.6)) | |
| incar[k] = mag | |
| elif k in ("LDAUU", "LDAUJ", "LDAUL"): | |
| mag.append(setting.get(site.specie.symbol, 0.6)) | |
| incar[key] = mag | |
| elif key in ("LDAUU", "LDAUJ", "LDAUL"): | |
| if hubbard_u: | |
| if hasattr(structure[0], k.lower()): | |
| m = {site.specie.symbol: getattr(site, k.lower()) for site in structure} | |
| incar[k] = [m[sym] for sym in poscar.site_symbols] | |
| if hasattr(structure[0], key.lower()): | |
| m = {site.specie.symbol: getattr(site, key.lower()) for site in structure} | |
| incar[key] = [m[sym] for sym in poscar.site_symbols] | |
| # lookup specific LDAU if specified for most_electroneg atom | |
| elif most_electro_neg in v and isinstance(v[most_electro_neg], dict): | |
| incar[k] = [v[most_electro_neg].get(sym, 0) for sym in poscar.site_symbols] | |
| elif most_electro_neg in setting and isinstance(setting[most_electro_neg], dict): | |
| incar[key] = [setting[most_electro_neg].get(sym, 0) for sym in poscar.site_symbols] | |
| # else, use fallback LDAU value if it exists | |
| else: | |
| incar[k] = [ | |
| v.get(sym, 0) if isinstance(v.get(sym, 0), (float, int)) else 0 | |
| incar[key] = [ | |
| setting.get(sym, 0) if isinstance(setting.get(sym, 0), (float, int)) else 0 | |
| for sym in poscar.site_symbols | |
| ] | |
| elif k.startswith("EDIFF") and k != "EDIFFG": | |
| if "EDIFF" not in settings and k == "EDIFF_PER_ATOM": | |
| incar["EDIFF"] = float(v) * len(structure) | |
| elif key.startswith("EDIFF") and key != "EDIFFG": | |
| if "EDIFF" not in settings and key == "EDIFF_PER_ATOM": | |
| incar["EDIFF"] = float(setting) * len(structure) | |
| else: | |
| incar["EDIFF"] = float(settings["EDIFF"]) | |
| elif k == "KSPACING" and v == "auto": | |
| elif key == "KSPACING" and setting == "auto": | |
| # default to metal if no prev calc available | |
| bandgap = 0 if self.bandgap is None else self.bandgap | |
| incar[k] = auto_kspacing(bandgap, self.bandgap_tol) | |
| incar[key] = auto_kspacing(bandgap, self.bandgap_tol) | |
| else: | |
| incar[k] = v | |
| incar[key] = setting | |
| _apply_incar_updates(incar, settings, structure, poscar, hubbard_u, most_electro_neg) |
| @@ -1013,7 +1013,7 @@ def override_from_prev_calc(self, prev_calc_dir="."): | |||
|
|
|||
| if getattr(self, "copy_wavecar", False): | |||
| for fname in ("WAVECAR", "WAVEDER", "WFULL"): | |||
| wavecar_files = sorted(glob(str(Path(prev_calc_dir) / (fname + "*")))) | |||
| wavecar_files = sorted(glob(str(Path(prev_calc_dir) / (f"{fname}*")))) | |||
There was a problem hiding this comment.
Ensure file existence checks before attempting to copy in override_from_prev_calc.
+ if os.path.exists(wavecar_file):
+ files_to_transfer[fname] = str(wavecar_files[-1])Before attempting to copy the WAVECAR files, it's crucial to check if the files exist to avoid runtime errors. This change ensures that the file exists before it's added to the files_to_transfer dictionary.
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.
| wavecar_files = sorted(glob(str(Path(prev_calc_dir) / (f"{fname}*")))) | |
| wavecar_files = sorted(glob(str(Path(prev_calc_dir) / (f"{fname}*")))) | |
| if os.path.exists(wavecar_file): | |
| files_to_transfer[fname] = str(wavecar_files[-1]) |
| vsfile = vsfile_fullpath if vsfile_fullpath in vruns else max(vruns) | ||
| outcarfile = outcarfile_fullpath if outcarfile_fullpath in outcars else max(outcars) |
There was a problem hiding this comment.
Handle potential exceptions when accessing files in get_vasprun_outcar.
- vsfile = vsfile_fullpath if vsfile_fullpath in vruns else max(vruns)
- outcarfile = outcarfile_fullpath if outcarfile_fullpath in outcars else max(outcars)
+ try:
+ vsfile = vsfile_fullpath if vsfile_fullpath in vruns else max(vruns)
+ outcarfile = outcarfile_fullpath if outcarfile_fullpath in outcars else max(outcars)
+ except ValueError:
+ raise FileNotFoundError("Could not find vasprun.xml or OUTCAR in the specified directory.")This change adds error handling to manage the case where vasprun.xml or OUTCAR files are not found in the specified directory, thus preventing the function from raising an unhandled ValueError.
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.
| vsfile = vsfile_fullpath if vsfile_fullpath in vruns else max(vruns) | |
| outcarfile = outcarfile_fullpath if outcarfile_fullpath in outcars else max(outcars) | |
| try: | |
| vsfile = vsfile_fullpath if vsfile_fullpath in vruns else max(vruns) | |
| outcarfile = outcarfile_fullpath if outcarfile_fullpath in outcars else max(outcars) | |
| except ValueError: | |
| raise FileNotFoundError("Could not find vasprun.xml or OUTCAR in the specified directory.") |
plus refactoring
FURB192: https://docs.astral.sh/ruff/rules/sorted-min-max