Rename sph_harm to sph_harm_y #413
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure WalkthroughUpdated minimum Python version to 3.10, replaced SciPy 1.9.3 with 1.15.0, and modernized type hints across 20+ modules from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
=======================================
Coverage 82.78% 82.78%
=======================================
Files 24 24
Lines 1807 1807
=======================================
Hits 1496 1496
Misses 311 311 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
structuretoolkit/build/sqs.py (2)
128-143: Update the docstring to match the modernized type hint.The function signature now uses PEP 604 syntax (
Atoms | np.ndarray), but the docstring at line 135 still references the oldUnionsyntax. Update the docstring for consistency.Apply this diff to update the docstring:
""" Remap the results of SQS optimization. Args: - result (Dict[str, Union[Atoms, np.ndarray]]): The result of SQS optimization. + result (dict[str, Atoms | np.ndarray]): The result of SQS optimization. Returns: - Tuple[Atoms, Dict[str, list]]: The remapped structure and short-range order parameters. + tuple[Atoms, dict[str, list]]: The remapped structure and short-range order parameters. """
160-242: Update the docstring to match the modernized type hints.The function signature uses PEP 604 syntax throughout (e.g.,
float | int,dict[int, float] | None), but the docstring at lines 184-202 still uses the oldOptionalandUnionsyntax. Update the docstring for consistency.Apply this diff to update the docstring:
""" Generate SQS structures. Args: structure (Atoms): The initial structure. - mole_fractions (Dict[str, Union[float, int]]): The mole fractions of each species. - weights (Optional[Dict[int, float]]): The weights for each shell. - objective (Union[float, np.ndarray]): The target objective value. - iterations (Union[float, int]): The number of iterations. + mole_fractions (dict[str, float | int]): The mole fractions of each species. + weights (dict[int, float] | None): The weights for each shell. + objective (float | np.ndarray): The target objective value. + iterations (float | int): The number of iterations. output_structures (int): The number of output structures. mode (str): The mode for selecting configurations. - num_threads (Optional[int]): The number of threads to use. - prefactors (Optional[Union[float, np.ndarray]]): The prefactors for each shell. - pair_weights (Optional[np.ndarray]): The pair weights. - rtol (Optional[float]): The relative tolerance. - atol (Optional[float]): The absolute tolerance. - which (Optional[Iterable[int]]): The indices of the shells to optimize. - shell_distances (Optional[Iterable[int]]): The distances for each shell. - minimal (Optional[bool]): Whether to minimize the objective function. - similar (Optional[bool]): Whether to generate similar structures. - return_statistics (Optional[bool]): Whether to return additional statistics. + num_threads (int | None): The number of threads to use. + prefactors (float | np.ndarray | None): The prefactors for each shell. + pair_weights (np.ndarray | None): The pair weights. + rtol (float | None): The relative tolerance. + atol (float | None): The absolute tolerance. + which (Iterable[int] | None): The indices of the shells to optimize. + shell_distances (Iterable[int] | None): The distances for each shell. + minimal (bool | None): Whether to minimize the objective function. + similar (bool | None): Whether to generate similar structures. + return_statistics (bool | None): Whether to return additional statistics. Returns: - Union[Atoms, Tuple[Atoms, Dict[str, list], int, float]]: The generated structures or a tuple containing the structures, short-range order parameters breakdown, number of iterations, and average cycle time. + Atoms | tuple[Atoms, dict[str, list], int, float]: The generated structures or a tuple containing the structures, short-range order parameters breakdown, number of iterations, and average cycle time. """
🧹 Nitpick comments (1)
structuretoolkit/visualize.py (1)
44-44: Consider refactoring mutable default arguments.Static analysis correctly flags the use of
np.array(...)in default arguments. While this specific case is low-risk (the arrays aren't mutated), it's a best practice to useNoneas the default and create the array inside the function.Apply this pattern:
def plot3d( ... - view_plane: np.ndarray = np.array([0, 0, 1]), + view_plane: np.ndarray | None = None, ... ): + if view_plane is None: + view_plane = np.array([0, 0, 1])Note: This is a pre-existing issue and not critical for this PR focused on type hint modernization.
Also applies to: 211-211
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.ci_support/environment-old.yml(1 hunks).github/workflows/pipeline.yml(1 hunks)pyproject.toml(1 hunks)structuretoolkit/analyse/distance.py(1 hunks)structuretoolkit/analyse/dscribe.py(1 hunks)structuretoolkit/analyse/neighbors.py(21 hunks)structuretoolkit/analyse/pyscal.py(4 hunks)structuretoolkit/analyse/snap.py(3 hunks)structuretoolkit/analyse/spatial.py(6 hunks)structuretoolkit/analyse/strain.py(4 hunks)structuretoolkit/analyse/symmetry.py(2 hunks)structuretoolkit/build/aimsgb.py(1 hunks)structuretoolkit/build/compound.py(5 hunks)structuretoolkit/build/sqs.py(2 hunks)structuretoolkit/build/surface.py(2 hunks)structuretoolkit/common/helper.py(2 hunks)structuretoolkit/visualize.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
structuretoolkit/analyse/neighbors.py (2)
structuretoolkit/analyse/spatial.py (1)
positions(331-341)structuretoolkit/analyse/strain.py (1)
num_neighbors(60-64)
🪛 Ruff (0.14.2)
structuretoolkit/analyse/dscribe.py
16-16: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
structuretoolkit/visualize.py
44-44: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
211-211: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: unittest_matrix (windows-latest, 3.13, win-64-py-3-13)
🔇 Additional comments (23)
structuretoolkit/build/aimsgb.py (1)
61-61: LGTM! Type annotation modernized correctly.The type hint has been updated to use PEP 604 syntax (
float | None), which is the preferred style for Python 3.10+. The parameter's deprecation handling remains intact.pyproject.toml (1)
14-25: LGTM! Python version requirements correctly updated.The minimum Python version has been appropriately raised to 3.10, and the classifiers have been updated to reflect support for Python 3.10-3.13. This aligns well with the type hint modernization throughout the codebase.
.github/workflows/pipeline.yml (1)
152-152: LGTM! CI configuration aligned with new Python minimum.The unittest_old workflow now correctly tests against Python 3.10, matching the updated minimum version requirement in pyproject.toml.
structuretoolkit/analyse/distance.py (1)
7-8: LGTM! Type hints modernized to PEP 604 syntax.The update from
Optional[np.ndarray]tonp.ndarray | Noneis correct and aligns with Python 3.10+ best practices. The change is purely syntactic with no runtime impact.structuretoolkit/common/helper.py (2)
25-25: LGTM! Type annotation modernized.The update to
np.ndarray | Nonecorrectly modernizes the type hint to Python 3.10+ syntax.
277-277: LGTM! Union type modernized to PEP 604 syntax.The conversion from
Union[Atoms, list, tuple, np.ndarray, float]toAtoms | list | tuple | np.ndarray | floatis correct and more readable.structuretoolkit/build/surface.py (2)
13-16: LGTM! Type hints consistently modernized.All four optional list parameters have been correctly updated to use
list | Nonesyntax, maintaining semantic equivalence while adopting Python 3.10+ conventions.
100-103: LGTM! Matching type hint updates.The parameter type hints in
high_index_surfacecorrectly mirror the modernization applied toget_high_index_surface_info.structuretoolkit/analyse/dscribe.py (1)
7-12: LGTM! Type hints modernized correctly.The parameter type hints have been properly updated to use PEP 604 union syntax (
float | None,int | None,np.ndarray | None), consistent with Python 3.10+ conventions.Also applies to: 15-15, 19-19
structuretoolkit/analyse/pyscal.py (4)
4-4: LGTM! Import statement cleaned up.The removal of
Optionalfrom the import is appropriate since the file now uses PEP 604 union syntax throughout. KeepingAnyfor the return type on line 244 is correct.
27-30: LGTM! Parameter and return type hints modernized.The function signature has been properly updated to use
int | None,tuple | Nonefor parameters andtuple[np.ndarray] | tuple[np.ndarray, np.ndarray]for the return type, all following PEP 604 conventions.
83-83: LGTM! Return type correctly updated.The union return type
dict[str, int] | np.ndarrayproperly reflects the function's behavior and uses modern syntax.
244-244: LGTM! Return type modernized.The return type
int | Anycorrectly uses PEP 604 syntax while maintaining the flexibility needed for the conditional return behavior..ci_support/environment-old.yml (1)
14-14: SciPy 1.15.0 compatibility verified—no breaking changes affecting identified imports.Analysis of identified scipy imports against SciPy 1.15.0 release notes and deprecation history (1.9.3→1.15.0):
All six scipy functions/classes actively used in the codebase remain unchanged and available:
scipy.interpolate.interp1dscipy.sparse.coo_matrixscipy.spatial.transform.Rotationscipy.spatial.cKDTreescipy.spatial.{ConvexHull, Delaunay, Voronoi}scipy.special.{gamma, sph_harm_y}scipy.constants.physical_constantsNone of these functions appear in SciPy's breaking changes or removal lists for versions 1.9.3 through 1.15.0. The breaking changes in 1.15.0 (BSpline.integrate return type, stats.wilcoxon method handling) and the removed/deprecated APIs (integrate.quadrature/romberg, signal functions, sparse-array legacy methods) do not affect the codebase's usage.
structuretoolkit/build/compound.py (1)
9-9: LGTM! Type hint modernization is correct.The updates from
Optional[float]tofloat | Nonefollow PEP 604 union syntax introduced in Python 3.10. The changes are consistent across all function signatures and don't alter runtime behavior.Also applies to: 34-34, 84-84, 126-126, 187-187
structuretoolkit/analyse/symmetry.py (1)
265-265: LGTM! Consistent type hint updates.The method signatures correctly use
bool | Noneinstead ofOptional[bool], aligning with Python 3.10+ union syntax.Also applies to: 382-383
structuretoolkit/analyse/spatial.py (2)
4-4: Good modernization of Callable import.Importing
Callablefromcollections.abcinstead oftypingis the recommended approach as of Python 3.9+, andtyping.Callableis deprecated for removal in Python 3.14.
228-228: LGTM! Type hints correctly modernized.All nullable parameters now use PEP 604 union syntax consistently across the spatial analysis module.
Also applies to: 297-297, 410-410, 460-460, 693-695
structuretoolkit/analyse/strain.py (1)
31-31: LGTM! Type annotations properly updated.The strain analysis methods now use modern union syntax for optional parameters. Changes are consistent and correct.
Also applies to: 86-86, 147-147, 320-320
structuretoolkit/visualize.py (1)
5-5: LGTM! Visualization module type hints modernized.All optional parameters across the plotting functions now use
T | Nonesyntax. The changes are extensive but consistent and correct.Also applies to: 33-42, 47-47, 207-209, 214-214, 287-296, 579-580, 729-730
structuretoolkit/analyse/neighbors.py (2)
151-152: LGTM! Type hint modernization is correct.All type hints have been properly updated to use Python 3.10+ union syntax (
T | Noneinstead ofOptional[T]). The changes are consistent throughout the neighbors module and don't affect runtime behavior.Also applies to: 260-261, 280-282, 364-367, 433-437, 472-475, 517-520, 553-556, 649-650, 869-874, 949-954, 1010-1013, 1107-1113, 1152-1159, 1225-1227, 1282-1287, 1359-1368, 1398-1407
12-12: The main premise is incorrect—the code correctly implements thesph_harm_ymigration despite confusing variable naming.The parameter mapping is actually correct:
theta(azimuthal) andphi(polar) are swapped in the function call to matchsph_harm_y's expected signature(n, m, theta_polar, phi_azimuth). The code compensates for the opposite variable naming convention.However, valid ancillary concerns remain:
- No test coverage: Zero tests found for
get_spherical_harmonics()or spherical harmonic calculations. This should be addressed.- Variable naming confusion: While functionally correct, naming
thetafor azimuthal angle andphifor polar angle is opposite to whatsph_harm_yexpects, making the code harder to verify. Consider renaming variables for clarity (e.g.,azimuth/polar_angle) or adding an explanatory comment.- SciPy version:
scipy==1.16.3is pinned;sph_harm_yis available since scipy 1.15, so compatibility is not an issue.The migration itself is sound from a scientific standpoint—results should be equivalent to a correct
sph_harmimplementation when parameters are properly mapped.Likely an incorrect or invalid review comment.
structuretoolkit/analyse/snap.py (1)
72-72: LGTM! Type hints correctly modernized.The
weightsparameter type hints now use PEP 604 union syntax, correctly expressing that the parameter acceptslist,np.ndarray, orNone.Also applies to: 123-123, 661-661
| weighting: Optional[np.ndarray] = None, | ||
| weighting: np.ndarray | None = None, | ||
| average: str = "off", | ||
| compression: dict = None, |
There was a problem hiding this comment.
Fix incomplete type hint modernization.
Line 14 was missed during the type hint modernization. The compression parameter should use the same PEP 604 syntax as other parameters in this function.
Apply this diff to complete the modernization:
- compression: dict = None,
+ compression: dict | None = None,📝 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.
| compression: dict = None, | |
| compression: dict | None = None, |
🤖 Prompt for AI Agents
In structuretoolkit/analyse/dscribe.py around line 14, the type hint for the
parameter `compression` still uses the old Optional style (`dict = None`);
update it to PEP 604 union syntax consistent with the rest of the function by
changing the annotation to `dict | None` (and keep the default `= None`) so the
parameter reads `compression: dict | None = None`.
Summary by CodeRabbit