Breaking: all plot methods return plt.Axes
#3749
Conversation
…age(.filters->'')
Replace ternary `if` expression with `or` operator
…ead of ax: plt.Axes e.g. WulffShape.get_plotly, BztPlotter.plot_props, several util.plotting funcs
… keyword to ax and return ax instead of plt
WalkthroughThe recent updates in pymatgen involve refining function signatures, variable names, return types, and plot settings for improved usability and clarity. These changes enhance the overall user experience in materials science computations. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files skipped from review as they are similar to previous changes (2)
Additional comments not posted (3)
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: 13
Actionable comments outside the diff hunks (6)
pymatgen/io/vasp/sets.py (6)
Line range hint
2200-2200: Ensure thatMPStaticSetsets theNSWparameter to 0 for static calculations.- this.config_dict["INCAR"]["NSW"] = this.nsw; + this.config_dict["INCAR"]["NSW"] = 0; # Ensure no structural relaxations in static calculations
Line range hint
2500-2500: Ensure thatMPSOCSetcorrectly sets theLSORBITparameter for SOC calculations.- this.config_dict["INCAR"]["LSORBIT"] = this.lsorbit; + this.config_dict["INCAR"]["LSORBIT"] = true; # Ensure LSORBIT is always true for SOC calculations
Line range hint
2800-2800: Ensure thatMVLScanRelaxSetcorrectly sets theLASPHparameter for SCAN calculations.- this.config_dict["INCAR"]["LASPH"] = this.lasph; + this.config_dict["INCAR"]["LASPH"] = true; # Ensure LASPH is always true for SCAN calculations
Line range hint
3000-3000: Ensure thatMPAbsorptionSetcorrectly sets theNOMEGAparameter for optical calculations.- this.config_dict["INCAR"]["NOMEGA"] = this.nomega; + this.config_dict["INCAR"]["NOMEGA"] = 1000; # Ensure NOMEGA is set to a suitable value for optical calculations
Line range hint
3300-3300: Ensure thatMVLScanRelaxSetcorrectly sets theLASPHparameter for SCAN calculations.- this.config_dict["INCAR"]["LASPH"] = this.lasph; + this.config_dict["INCAR"]["LASPH"] = true; # Ensure LASPH is always true for SCAN calculations
Line range hint
3500-3500: Ensure thatMPAbsorptionSetcorrectly sets theNOMEGAparameter for optical calculations.- this.config_dict["INCAR"]["NOMEGA"] = this.nomega; + this.config_dict["INCAR"]["NOMEGA"] = 1000; # Ensure NOMEGA is set to a suitable value for optical calculations
| @@ -812,7 +812,7 @@ def get_magsymops(self, data): | |||
|
|
|||
| # else check to see if it specifies a magnetic space group | |||
| elif bns_name or bns_num: | |||
| label = bns_name if bns_name else list(map(int, (bns_num.split(".")))) | |||
| label = bns_name or list(map(int, (bns_num.split(".")))) | |||
There was a problem hiding this comment.
Refactor the assignment of label to use a more Pythonic approach.
- label = bns_name or list(map(int, (bns_num.split("."))))
+ label = bns_name if bns_name is not None else list(map(int, bns_num.split(".")))This change makes the conditional assignment clearer and more in line with common Python practices.
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.
| label = bns_name or list(map(int, (bns_num.split(".")))) | |
| label = bns_name if bns_name is not None else list(map(int, bns_num.split("."))) |
|
|
||
| Returns: |
There was a problem hiding this comment.
The method symm_group_cubic lacks a docstring. Adding a docstring would improve code maintainability by explaining the purpose and usage of this method.
+ """
+ Obtain cubic symmetric equivalents of the list of vectors.
+
+ Args:
+ mat (n by 3 array/matrix): lattice matrix
+
+ Returns:
+ cubic symmetric equivalents of the list of vectors.
+ """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.
| Returns: | |
| """ | |
| Obtain cubic symmetric equivalents of the list of vectors. | |
| Args: | |
| mat (n by 3 array/matrix): lattice matrix | |
| Returns: | |
| cubic symmetric equivalents of the list of vectors. | |
| """ |
|
|
||
| Returns: |
There was a problem hiding this comment.
The method reduce_mat lacks a docstring. Adding a docstring would improve code maintainability by explaining the purpose and usage of this method.
+ """
+ Reduce integer array mat's determinant mag times by linear combination
+ of its row vectors, so that the new array after rotation (r_matrix) is
+ still an integer array.
+
+ Args:
+ mat (3 by 3 array): input matrix
+ mag (int): reduce times for the determinant
+ r_matrix (3 by 3 array): rotation matrix
+ """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.
| Returns: | |
| """ | |
| Reduce integer array mat's determinant mag times by linear combination | |
| of its row vectors, so that the new array after rotation (r_matrix) is | |
| still an integer array. | |
| Args: | |
| mat (3 by 3 array): input matrix | |
| mag (int): reduce times for the determinant | |
| r_matrix (3 by 3 array): rotation matrix | |
| """ |
|
|
||
| Returns: |
There was a problem hiding this comment.
The method vec_to_surface lacks a docstring. Adding a docstring would improve code maintainability by explaining the purpose and usage of this method.
+ """
+ Transform a float vector to a surface miller index with integers.
+
+ Args:
+ vec (1 by 3 array float vector): input float vector
+
+ Returns:
+ the surface miller index of the input vector.
+ """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.
| Returns: | |
| """ | |
| Transform a float vector to a surface miller index with integers. | |
| Args: | |
| vec (1 by 3 array float vector): input float vector | |
| Returns: | |
| the surface miller index of the input vector. | |
| """ |
| @@ -121,7 +121,7 @@ def __init__( | |||
| """ | |||
| self.miller_index = miller_index | |||
| self.label = label | |||
| self.adsorbates = adsorbates if adsorbates else [] | |||
| self.adsorbates = adsorbates or [] | |||
There was a problem hiding this comment.
Consider using if adsorbates is None instead of or for clarity and to avoid unintended behavior if adsorbates is an empty list but not None.
- self.adsorbates = adsorbates or []
+ self.adsorbates = [] if adsorbates is None else adsorbatesCommittable 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.
| self.adsorbates = adsorbates or [] | |
| self.adsorbates = [] if adsorbates is None else adsorbates |
Ensure proper handling of default values for adsorbates to avoid potential bugs with falsy but valid values.
- self.adsorbates = adsorbates or []
+ self.adsorbates = [] if adsorbates is None else adsorbatesCommittable 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.
| self.adsorbates = adsorbates or [] | |
| self.adsorbates = [] if adsorbates is None else adsorbates |
| @@ -180,7 +180,7 @@ | |||
| float: The surface energy of the slab. | |||
| """ | |||
| # Set up | |||
| ref_entries = ref_entries if ref_entries else [] | |||
| ref_entries = ref_entries or [] | |||
There was a problem hiding this comment.
Use explicit None check for ref_entries to ensure clarity and prevent errors with other falsy values.
- ref_entries = ref_entries or []
+ ref_entries = [] if ref_entries is None else ref_entriesCommittable 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.
| ref_entries = ref_entries or [] | |
| ref_entries = [] if ref_entries is None else ref_entries |
Explicitly handle None values for ref_entries to avoid potential issues with falsy but non-None values.
- ref_entries = ref_entries or []
+ ref_entries = [] if ref_entries is None else ref_entriesCommittable 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.
| ref_entries = ref_entries or [] | |
| ref_entries = [] if ref_entries is None else ref_entries |
Ensure ref_entries is handled correctly by explicitly checking for None.
- ref_entries = ref_entries or []
+ ref_entries = [] if ref_entries is None else ref_entriesCommittable 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.
| ref_entries = ref_entries or [] | |
| ref_entries = [] if ref_entries is None else ref_entries |
To prevent potential logical errors, explicitly check for None when assigning default values.
- ref_entries = ref_entries or []
+ ref_entries = [] if ref_entries is None else ref_entriesCommittable 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.
| ref_entries = ref_entries or [] | |
| ref_entries = [] if ref_entries is None else ref_entries |
| @@ -1794,7 +1794,7 @@ def parse_struct(path_dir): | |||
| path_dir: (str) dir containing the boltztrap.struct file | |||
|
|
|||
| Returns: | |||
| (float) volume | |||
| float: volume of the structure in Angstrom^3 | |||
There was a problem hiding this comment.
The function parse_struct should explicitly return a float type for the volume as indicated in the comment. Ensure the return type is consistent with the documentation.
- return Lattice(
+ return float(Lattice(
[[Length(float(tokens[i].split()[j]), "bohr").to("ang") for j in range(3)] for i in range(1, 4)]
).volume)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.
| float: volume of the structure in Angstrom^3 | |
| return float(Lattice( | |
| [[Length(float(tokens[i].split()[j]), "bohr").to("ang") for j in range(3)] for i in range(1, 4)] | |
| ).volume) |
| n_atoms = 0 | ||
| for idx, line in enumerate(file): | ||
| line = line.rstrip("\n") | ||
| if idx == 0 and "CUBE" not in line: | ||
| raise ValueError("CUBE file format not recognized") | ||
|
|
||
| if idx == 2: | ||
| tokens = line.split() | ||
| natoms = int(tokens[0]) | ||
| n_atoms = int(tokens[0]) |
There was a problem hiding this comment.
The variable n_atoms is initialized but not used within the read_cube_file function. Consider removing it if it's unnecessary, or ensure it's used appropriately if needed.
- n_atoms = 0
- for idx, line in enumerate(file):
- line = line.rstrip("\n")
- if idx == 0 and "CUBE" not in line:
- raise ValueError("CUBE file format not recognized")
-
- if idx == 2:
- tokens = line.split()
- n_atoms = int(tokens[0])
- if idx == 3:
+ for idx, line in enumerate(file):
+ line = line.rstrip("\n")
+ if idx == 0 and "CUBE" not in line:
+ raise ValueError("CUBE file format not recognized")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.
| n_atoms = 0 | |
| for idx, line in enumerate(file): | |
| line = line.rstrip("\n") | |
| if idx == 0 and "CUBE" not in line: | |
| raise ValueError("CUBE file format not recognized") | |
| if idx == 2: | |
| tokens = line.split() | |
| natoms = int(tokens[0]) | |
| n_atoms = int(tokens[0]) | |
| for idx, line in enumerate(file): | |
| line = line.rstrip("\n") | |
| if idx == 0 and "CUBE" not in line: | |
| raise ValueError("CUBE file format not recognized") |
| energy_data = np.genfromtxt(filename, skip_header=n_atoms + 6, skip_footer=1) | ||
| n_lines_data = len(energy_data) | ||
| last_line = np.genfromtxt(filename, skip_header=n_lines_data + natoms + 6) | ||
| last_line = np.genfromtxt(filename, skip_header=n_lines_data + n_atoms + 6) |
There was a problem hiding this comment.
The variable n_lines_data is used to calculate the last_line but is not necessary. You can simplify the code by directly using the skip_header parameter with the appropriate value.
- n_lines_data = len(energy_data)
- last_line = np.genfromtxt(filename, skip_header=n_lines_data + n_atoms + 6)
+ last_line = np.genfromtxt(filename, skip_header=len(energy_data) + n_atoms + 6)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.
| energy_data = np.genfromtxt(filename, skip_header=n_atoms + 6, skip_footer=1) | |
| n_lines_data = len(energy_data) | |
| last_line = np.genfromtxt(filename, skip_header=n_lines_data + natoms + 6) | |
| last_line = np.genfromtxt(filename, skip_header=n_lines_data + n_atoms + 6) | |
| energy_data = np.genfromtxt(filename, skip_header=n_atoms + 6, skip_footer=1) | |
| last_line = np.genfromtxt(filename, skip_header=len(energy_data) + n_atoms + 6) |
a few plotting methods were overlooked in the
return plttoreturn axmigration #3237.this PR makes
pymatgen's plotting API consistent by returningplt.Axesfrom all plotting methodsalso fixes the return types in many doc strings and adds
Returns:section to several doc strings that were missing themSummary by CodeRabbit
Documentation
Refactor
New Features
get_plotfunction to return a 3D plot of the Wulff shape.Style