Imperative doc strings #3792
Imperative doc strings #3792
Conversation
janosh
added
linting
WalkthroughThe recent updates across various modules in the Changes
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 (
|
| """ | ||
| if not isinstance(other, Composition): | ||
| return NotImplemented | ||
|
|
||
| for el in sorted(set(self.elements + other.elements)): | ||
| if other[el] - self[el] >= Composition.amount_tolerance: | ||
| return False | ||
| # TODO @janosh 2024-04-29: is this a bug? why would we return True early? |
There was a problem hiding this comment.
@DanielYang59 @JaGeo curious if anyone else thinks this early return True in comp1 >= comp2 looks strange? how can we return before having looped through the whole list of elements?
There was a problem hiding this comment.
Looks like a bug to me. Only one of the elements is "greater than" doesn't necessarily mean the Composition (set of elements) is greater than. Should check the full set.
I have another question though: the implementation seems to check every element in the union of self and other, what happens when the element is only in one of them? Would there be an IndexError? Should we capture the error and return False? (For example H2O >= Fe3O4?)
Also I didn't see much good reason to sort this set?
Finally I assume it might be good to clarify Should ONLY be used for defining a sort order (the behavior is probably not what you'd expect). or remove it altogether to avoid confusion? As you have already clarified the mechanism in the docstring.
There was a problem hiding this comment.
thanks for confirming. we can fix it in a subsequent PR to keep this one restricted to doc strings
![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
Out of diff range and nitpick comments (36)
pymatgen/analysis/ewald.py (4)
Line range hint
62-62: Consider adding error handling for division by zero in the_charged_cell_energycalculation to prevent runtime errors ifstructure.volumeorself._etais zero.
Line range hint
95-95: Optimize the energy summation by usingnumpy.sum(total_energy_matrix)instead ofsum(sum(total_energy_matrix))for better performance.
Line range hint
104-104: Enhance the clarity of error messages and add more detailed comments in thecompute_sub_structuremethod to improve maintainability and understandability.
Line range hint
273-273: Consider refactoring the_calc_ewald_termsmethod to split it into smaller, more manageable functions. This would enhance readability and maintainability.pymatgen/analysis/adsorption.py (3)
Line range hint
50-50: Consider adding input validation for the parametersslab,selective_dynamics,height, andmi_vecin the__init__method to ensure they meet expected types and constraints.
Line range hint
250-250: Optimize thefind_adsorption_sitesmethod by reducing redundant calculations and utilizing numpy operations for better performance and clarity.
Line range hint
624-624: Add error handling for division by zero in theget_mi_vecfunction to prevent runtime errors when the cross product of lattice vectors is zero.pymatgen/analysis/pourbaix_diagram.py (1)
Line range hint
755-774: Refactor the method to improve clarity and reduce complexity.- return self.energy + self.npH * PREFAC * pH + self.nPhi * V + free_energy = self.energy + self.npH * PREFAC * pH + self.nPhi * V + return free_energyThis change introduces an intermediate variable
free_energyto make the calculation steps clearer and easier to follow.pymatgen/io/lammps/inputs.py (8)
47-47: Ensure consistent documentation style.The docstring for the class
LammpsInputFileshould follow a consistent style. Consider using imperative mood consistently across all docstrings for uniformity and clarity.
Line range hint
63-63: Check for potential performance issues.The uniqueness check for stage names (
if len(self.stages_names) != len(set(self.stages_names))) is performed every time an instance is created. This could be inefficient for a large number of stages. Consider performing this check only when stages are added or modified.
Line range hint
217-217: Clarify method parameters.The parameter
commandsin theadd_stagemethod can accept multiple types (str, list, dict). It would be beneficial to clarify this in the docstring to ensure that users are aware of the flexibility of this parameter.
Line range hint
365-365: Enhance error handling.The method
remove_commandcould improve its error handling by providing more specific messages when a command is not found in the specified stages. This would help users understand why a removal operation failed.
Line range hint
485-485: Consider adding more flexibility.The method
get_strcould be enhanced by adding more formatting options, such as custom separators between stages or commands. This would allow users to generate more customized outputs based on their specific needs.
Line range hint
631-631: Update documentation for clarity.The docstring for the constructor of
LammpsRuncould be improved by specifying what types of placeholders are expected inscript_templateand how they are replaced. This would help users understand how to properly format their templates.
Line range hint
731-731: Ensure method documentation is comprehensive.The method
get_input_setin theLammpsTemplateGenclass lacks a detailed explanation of its parameters, especiallydata_filename. Expanding the docstring to include descriptions of each parameter would enhance clarity and usability.
Line range hint
785-785: Improve error handling and user feedback.The function
write_lammps_inputsshould handle potential errors during file operations more robustly. For instance, it could catch exceptions related to file writing and provide more informative error messages to the user.pymatgen/analysis/structure_matcher.py (3)
Line range hint
36-38: Consider adding type hints for thecompositionparameter in theget_hashmethod to improve code clarity and consistency.- def get_hash(self, composition): + def get_hash(self, composition: Composition):
Line range hint
49-62: Potential security risk: dynamic class instantiation based on untrusted input can lead to arbitrary code execution. Ensure that the input tofrom_dictis sanitized or consider safer alternatives.
Line range hint
141-156: Optimization suggestion: The nested loops inare_equalcan be inefficient for large species dictionaries. Consider using more efficient data structures or algorithms to improve performance.pymatgen/io/abinit/abiobjects.py (8)
Line range hint
33-33: Consider adding type hints for the function parameters to improve code readability and maintainability.
Line range hint
33-33: The function lacks a detailed docstring for theargsandkwargsparameters. It would be beneficial for maintainability and usability to document these, especially since they play a critical role in the function's behavior.
Line range hint
44-44: The error message "angdeg and rprimd are mutually exclusive" contains a typo: it should berpriminstead ofrprimd.- raise ValueError("angdeg and rprimd are mutually exclusive") + raise ValueError("angdeg and rprim are mutually exclusive")
Line range hint
75-75: The error message uses f-string with an expression directly in the format ({ang_deg=}). This is not supported in all versions of Python. Consider using a more compatible formatting method.- raise ValueError(f"The sum of angdeg must be lower than 360, {ang_deg=}") + raise ValueError("The sum of angdeg must be lower than 360, ang_deg={}".format(ang_deg))
Line range hint
107-107: The functionstructure_from_abivarscould benefit from type hints for its parameters to enhance code clarity and assist with type checking.
Line range hint
107-107: The docstring forstructure_from_abivarsshould include descriptions forargsandkwargssince they are critical to the function's execution but are not documented.
Line range hint
129-129: The error message "xangst and xcart are mutually exclusive" could be more informative by specifying that these are coordinate types and only one can be used at a time.- raise ValueError("xangst and xcart are mutually exclusive") + raise ValueError("Coordinate types 'xangst' and 'xcart' are mutually exclusive and cannot be used together.")
Line range hint
183-183: The functionspecies_by_znuclis well-implemented with a clear purpose. However, adding a more detailed docstring explaining the importance of the order and why the set is not used could help future maintainers or users of the code.pymatgen/io/qchem/outputs.py (9)
687-687: Consider adding a docstring to the_read_fock_matrixmethod to improve code documentation and maintainability.
726-726: Consider adding a docstring to the_read_coefficient_matrixmethod to enhance clarity and maintainability.
764-764: Consider adding a docstring to the_read_charge_and_multiplicitymethod to enhance clarity and maintainability.
798-798: Consider adding a docstring to the_read_species_and_inital_geometrymethod to enhance clarity and maintainability.
840-840: Consider adding a docstring to the_read_SCFmethod to enhance clarity and maintainability.
1769-1769: Consider adding a docstring to the_read_pcm_informationmethod to enhance clarity and maintainability.
1794-1794: Consider adding a docstring to the_read_smd_informationmethod to enhance clarity and maintainability.
1911-1911: Consider adding a docstring to the_read_nbo_datamethod to enhance clarity and maintainability.
1919-1919: Consider adding a docstring to the_read_cdftmethod to enhance clarity and maintainability.
| """Composition greater than or equal to. We consider compositions A >= B | ||
| if all elements in B are in A and the amount of each element in A is | ||
| greater than or equal to the amount of the element in B within | ||
| Composition.amount_tolerance. | ||
|
|
||
| Should ONLY be used for defining a sort order (the behavior is probably not what you'd expect). | ||
| """ | ||
| if not isinstance(other, Composition): | ||
| return NotImplemented | ||
|
|
||
| for el in sorted(set(self.elements + other.elements)): | ||
| if other[el] - self[el] >= Composition.amount_tolerance: | ||
| return False | ||
| # TODO @janosh 2024-04-29: is this a bug? why would we return True early? |
There was a problem hiding this comment.
Refine the logic in the __ge__ method to prevent early returns.
- if self[el] - other[el] >= Composition.amount_tolerance:
- return TrueThe early return in the __ge__ method (line 208) might lead to incorrect results if not all elements have been checked yet. It's generally safer to complete all checks before returning a result.
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.
| """Composition greater than or equal to. We consider compositions A >= B | |
| if all elements in B are in A and the amount of each element in A is | |
| greater than or equal to the amount of the element in B within | |
| Composition.amount_tolerance. | |
| Should ONLY be used for defining a sort order (the behavior is probably not what you'd expect). | |
| """ | |
| if not isinstance(other, Composition): | |
| return NotImplemented | |
| for el in sorted(set(self.elements + other.elements)): | |
| if other[el] - self[el] >= Composition.amount_tolerance: | |
| return False | |
| # TODO @janosh 2024-04-29: is this a bug? why would we return True early? | |
| """Composition greater than or equal to. We consider compositions A >= B | |
| if all elements in B are in A and the amount of each element in A is | |
| greater than or equal to the amount of the element in B within | |
| Composition.amount_tolerance. | |
| Should ONLY be used for defining a sort order (the behavior is probably not what you'd expect). | |
| """ | |
| if not isinstance(other, Composition): | |
| return NotImplemented | |
| for el in sorted(set(self.elements + other.elements)): | |
| if other[el] - self[el] >= Composition.amount_tolerance: | |
| return False | |
| # TODO @janosh 2024-04-29: is this a bug? why would we return True early? |
| @@ -680,7 +680,7 @@ def set_temperature(self, temperature: float) -> None: | |||
| dof = 3 * len(self.structure) - 3 | |||
|
|
|||
| # Remove linear drift (net momentum) | |||
| velocities -= np.average(atomic_masses[:, np.newaxis] * velocities, axis=0) / np.average(atomic_masses) | |||
| velocities -= np.mean(atomic_masses[:, np.newaxis] * velocities, axis=0) / np.mean(atomic_masses) | |||
There was a problem hiding this comment.
Consider using a more efficient method for string concatenation.
- potcar_to_hash_str = "\n".join(potcar_to_hash)
+ potcar_to_hash_str = ''.join(f"{line}\n" for line in potcar_to_hash)This change avoids creating an intermediate list and directly constructs the string, which can be more memory efficient.
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.
| velocities -= np.mean(atomic_masses[:, np.newaxis] * velocities, axis=0) / np.mean(atomic_masses) | |
| velocities -= np.mean(atomic_masses[:, np.newaxis] * velocities, axis=0) / np.mean(atomic_masses) |
|
|
||
| # Number of valence electrons including possible extra charge | ||
| n_val_elec = num_valence_electrons(structure, pseudos) | ||
| n_val_elec -= electrons.charge | ||
|
|
||
| # First guess (semiconductors) | ||
| nband = n_val_elec // 2 | ||
| n_band = n_val_elec // 2 |
There was a problem hiding this comment.
Consider addressing the TODO comment to improve the algorithm for calculating n_band.
Would you like me to help with developing a more robust algorithm for this calculation?
| @@ -979,7 +979,7 @@ def get_pourbaix_plot( | |||
| ax.plot(V0_line[0], V0_line[1], "k-.", linewidth=lw) | |||
|
|
|||
| for entry, vertices in self._pbx._stable_domain_vertices.items(): | |||
| center = np.average(vertices, axis=0) | |||
| center = np.mean(vertices, axis=0) | |||
There was a problem hiding this comment.
Ensure consistent formatting in the as_dict method.
- dct = {"@module": type(self).__module__, "@class": type(self).__name__}
+ dct = {
+ "@module": type(self).__module__,
+ "@class": type(self).__name__
+ }This change improves the readability of the dictionary initialization by aligning the keys and values.
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.
| center = np.mean(vertices, axis=0) | |
| center = np.mean(vertices, axis=0) |
Consider adding error handling in the __getattr__ method.
+ if attr not in self.__dict__:
+ raise AttributeError(f"{type(self).__name__} object has no attribute '{attr}'")This addition prevents the method from failing silently if an attribute is not found, providing a clearer error message to the user.
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.
| center = np.mean(vertices, axis=0) | |
| center = np.mean(vertices, axis=0) |
| @@ -2511,7 +2511,7 @@ def get_sorted_structure(self, key=None, reverse=False) -> Structure: | |||
| return struct_copy | |||
|
|
|||
| def get_shifts_based_on_adsorbate_sites(self, tolerance: float = 0.1) -> list[tuple[float, float]]: | |||
| """Computes possible in-plane shifts based on an adsorbate site algorithm. | |||
| """Compute possible in-plane shifts based on an adsorbate site algorithm. | |||
There was a problem hiding this comment.
The method get_shifts_based_on_adsorbate_sites lacks a return statement.
Please ensure that the method returns the computed shifts or handles the case where no shifts are found.
The constructor of Interface class should validate the types and values of the input parameters, especially for the new interface-specific properties.
Consider adding checks to ensure that in_plane_offset is a tuple of two floats, gap is a non-negative float, and vacuum_over_film is also a non-negative float.
| @@ -621,7 +621,7 @@ def best_case(self, matrix, m_list, indices_left): | |||
| elapsed_time = datetime.utcnow() - self._start_time | |||
| speedup_parameter = elapsed_time.total_seconds() / 1800 | |||
| avg_int = np.sum(interaction_matrix, axis=None) | |||
| avg_frac = np.average(np.outer(1 - fractions, 1 - fractions)) | |||
| avg_frac = np.mean(np.outer(1 - fractions, 1 - fractions)) | |||
There was a problem hiding this comment.
Add explicit checks for the presence of necessary attributes (oxi_state and charge) in the compute_average_oxidation_state function to prevent potential AttributeError exceptions.
| @@ -139,7 +139,7 @@ def from_bulk_and_miller( | |||
| surf_props, under_coords = [], [] | |||
| this_mi_vec = get_mi_vec(this_slab) | |||
| mi_mags = [np.dot(this_mi_vec, site.coords) for site in this_slab] | |||
| average_mi_mag = np.average(mi_mags) | |||
| average_mi_mag = np.mean(mi_mags) | |||
There was a problem hiding this comment.
Simplify the list comprehension in the find_surface_sites_by_height method to enhance readability and maintainability.
|
|
||
| return np.average([site_list[idx].frac_coords for idx in indices], axis=0) | ||
| return np.mean([site_list[idx].frac_coords for idx in indices], axis=0) |
There was a problem hiding this comment.
Add error handling in the add_adsorbate method to ensure that the molecule has the expected properties and coordinates before proceeding with the adsorption process.
Optimize the get_rot function by pre-computing values and reducing the number of operations to enhance performance and clarity.
Optimize the put_coord_inside function by utilizing numpy operations for better performance and clarity.
pymatgen/io/vasp/sets.py
Outdated
| @@ -2128,7 +2128,7 @@ def incar_updates(self) -> dict: | |||
| updates.update({"AMIN": 0.01, "AMIX": 0.2, "BMIX": 0.001}) | |||
| if self.auto_dipole: | |||
| weights = [s.species.weight for s in self.structure] # type: ignore | |||
| center_of_mass = np.average(self.structure.frac_coords, weights=weights, axis=0) # type: ignore | |||
| center_of_mass = np.mean(self.structure.frac_coords, weights=weights, axis=0) # type: ignore | |||
There was a problem hiding this comment.
Typographical error in the comment.
-# TODO throw error for Ultrasoft potentials
+# TODO: Throw error for Ultrasoft potentials.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.
| center_of_mass = np.mean(self.structure.frac_coords, weights=weights, axis=0) # type: ignore | |
| center_of_mass = np.mean(self.structure.frac_coords, weights=weights, axis=0) # TODO: Throw error for Ultrasoft potentials. |
| @@ -73,8 +73,7 @@ def __str__(self): | |||
|
|
|||
|
|
|||
| class ExplicitPermutationsAlgorithm(AbstractChemenvAlgorithm): | |||
| """ | |||
| Class representing the algorithm doing the explicit permutations for the calculation of | |||
| """Algorithm doing the explicit permutations for the calculation of | |||
There was a problem hiding this comment.
Thanks for these docstring improvement. I was thinking the same.
The keyword class should already be self-documenting and no need to repeat it's "Class for some purpose."
Also as per PEP-8 the docstring should usually start with a brief high-level summary on the first line (the line with ```), so I totally support your changes here!
| @@ -107,7 +107,7 @@ def generate_points(coord_left: int = -10, coord_right: int = 10) -> np.ndarray: | |||
| def zone_axis_filter( | |||
| self, points: list[tuple[int, int, int]] | np.ndarray, laue_zone: int = 0 | |||
| ) -> list[tuple[int, int, int]]: | |||
| """Filters out all points that exist within the specified Laue zone according to the zone axis rule. | |||
| """Filter out all points that exist within the specified Laue zone according to the zone axis rule. | |||
There was a problem hiding this comment.
I noticed some similar issues (and thought pymatgen has a different convention than others).
But it appears Google-styled docstring doesn't really have an explicit preference.
|
I hope I'm not caring too much about nitty gritty details, but I have a feeling that the pymatgen/pymatgen/core/composition.py Lines 290 to 340 in 9964c4a |
i agree |
I'm afraid there is some misunderstanding. I thought the docstring of a For example: @property
def labels(self):
- """Get a dictionary containing kink information:
+ """A dictionary containing kink information:What do you think? |
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
pymatgen/analysis/interface_reactions.py (1)
566-566: Validate the format and content of labels in thelabelsproperty.The format string in the
labelsproperty uses a fixed number of decimal places forxandenergy. Consider whether this level of precision is appropriate for all use cases, or if it should be configurable based on user input or the specific requirements of the application.
| @@ -207,8 +207,7 @@ def plot(self, backend: Literal["plotly", "matplotlib"] = "plotly") -> Figure | | |||
| return fig | |||
|
|
|||
| def get_dataframe(self) -> DataFrame: | |||
| """ | |||
| Returns a pandas DataFrame representation of the data produced by the | |||
| """Get a pandas DataFrame representation of the data produced by the | |||
There was a problem hiding this comment.
Ensure the docstring accurately reflects the method's functionality.
The docstring for get_dataframe method should clearly describe what the DataFrame contains and how it is structured. Consider adding details about the columns and the type of data each column holds.
| @@ -224,8 +223,7 @@ def get_dataframe(self) -> DataFrame: | |||
| return DataFrame(rxns) | |||
|
|
|||
| def get_critical_original_kink_ratio(self): | |||
| """ | |||
| Returns a list of molar mixing ratio for each kink between ORIGINAL | |||
| """Get a list of molar mixing ratio for each kink between ORIGINAL | |||
There was a problem hiding this comment.
Clarify the purpose and usage of the get_critical_original_kink_ratio method in the docstring.
The docstring for get_critical_original_kink_ratio should explain why one might need the molar mixing ratios for the original reactant compositions and in what scenarios this method is particularly useful.
| @@ -243,8 +241,7 @@ def get_critical_original_kink_ratio(self): | |||
| return ratios | |||
|
|
|||
| def _get_original_composition_ratio(self, reaction): | |||
| """ | |||
| Returns the molar mixing ratio between the reactants with ORIGINAL ( | |||
| """Get the molar mixing ratio between the reactants with ORIGINAL ( | |||
There was a problem hiding this comment.
Improve clarity of the _get_original_composition_ratio method's docstring.
The docstring for _get_original_composition_ratio should include more details about how the molar mixing ratio is calculated and the significance of the original compositions in the context of the reaction.
no misunderstanding, that's my goal as well. there may have been some mass find and replace changes where this was not the end result. but i'll get to those in the 2nd pass |
… have been normal method all along
Beautiful! Thanks for the improvements. Meanwhile maybe can we set the CodeRabbit to review only when we explicitly call it (like |
very much so! i asked @shyuep if we could disable or configure it differently. e.g. would be nice if it ignored PRs from maintainers |
|
There is. Coderabbit.yaml file that can be edited in the repo. |
This reverts commit cae7aed.
move dunder methods to the top add more types and tweaks relocate more dunder methods to top more types and format tweaks fix type error add types for composition help fix materialsproject#3792 (comment) reverse compare order for readability Revert "reverse compare order for readability" This reverts commit 05ea23a. Revert "help fix materialsproject#3792 (comment)" This reverts commit cae7aed. add types for `core.bonds` finish `core.ion` add some types revert non-interface changes
…cfunc`, new type `MillerIndex` and fix Lattice hash (#3814) * tweak type and docstring * move dunder methods to the top * add more types and tweaks * relocate more dunder methods to top * more types and format tweaks * fix type error * add types for composition * help fix #3792 (comment) * reverse compare order for readability * Revert "reverse compare order for readability" This reverts commit 05ea23a. * Revert "help fix #3792 (comment)" This reverts commit cae7aed. * add types for `core.bonds` * finish `core.ion` * add some types * revert changes on core.interface * add types for `libxfunc` * remove unnecessary `libxc_version` * recover header * finish `Lattice` * fix unit test * Revert "fix unit test" This reverts commit 4d17bcd. * merge two pbc checks * improve property pbc setter * fix type for pbc * fix circular import * revert script (unsure if needed) * add new type `MillerIndex` * change types to `MillerIndex` * add a proper hash method * fix typo in composition check of OH * revert non-interface changes * Revert "revert non-interface changes" This reverts commit 7930238. * refactor * tweak doc strings --------- Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
* tweak type and docstring move dunder methods to the top add more types and tweaks relocate more dunder methods to top more types and format tweaks fix type error add types for composition help fix #3792 (comment) reverse compare order for readability Revert "reverse compare order for readability" This reverts commit 05ea23a. Revert "help fix #3792 (comment)" This reverts commit cae7aed. add types for `core.bonds` finish `core.ion` add some types revert non-interface changes * use math.gcd over gcd * use more specific types for ClassVar * more: use more specific types for ClassVar * fix some type errors and comment tweaks * fix mypy errors * enable types: more type errors to fix * fix type errors * fix type errors * fix mypy errors doesn't show locally * revert change in test and convert rotation_axis and plane to tuple * cast plane type to tuple * remove `del` of var name * add and update new type `Tuple3Ints = tuple[int, int, int]` * relocate `Tuple4Ints` to `core.interface` * relocate `Tuple4Ints` * use `Tuple3Floats` * revert usage of assert_allclose * use more meaningful types * fix replacement * Revert "fix replacement" This reverts commit 6b9589a. * revert type aliases in docstring
follow up to #3790