Fix ruff by sorting the class variables in _so3.py #441
Conversation
Signed-off-by: Tsz Wai Ko <[email protected]>
…taining and using element offsets
…ction including both
…the update of sympy
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tsz Wai Ko <[email protected]>
Signed-off-by: Tsz Wai Ko <[email protected]>
Bumps [boto3](https://github.com/boto/boto3) from 1.35.38 to 1.35.39. - [Release notes](https://github.com/boto/boto3/releases) - [Commits](boto/boto3@1.35.38...1.35.39) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
* improve TensorNet model coverage * Update pyproject.toml Signed-off-by: Tsz Wai Ko <[email protected]> * Improve the unit test for SO(3) equivarance in TensorNet class * improve SO3Net model class coverage and simplify TensorNet implementations * improve the coverage in MLP_norm class * Improve the implementation of three-body interactions * fixed black * Optimize the speed of _compute_3body class * type checking is added for scheduler * update M3GNet Potential training notebook for the demonstration of obtaining and using element offsets * Downgrade sympy to avoid crash of SO3 operations * Smooth l1 loss function is added and united tests are improved * merge the method predict_structure and featurize_structure into a function including both * remove unnecessary else statement for training magmoms * modify so3 operation implementation to make united tests pass due to the update of sympy * skip test_load_all_models for MacOS pytest now * Reference for CHGNet is added * Update README.md and index.md for including CHGNet Signed-off-by: Tsz Wai Ko <[email protected]> * add more description for using CHGNet pretrained models in Relaxations and Simulations using the M3GNet Universal Potential.ipynb * A command-line interface for performing ASE MD simulations is added * added back py.typed * ExpNormal Smearing for radial basis functions is added * Changed deprecated torch.scalar_tensor into torch.Tensor Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tsz Wai Ko <[email protected]> * Converted the float number into tensor Signed-off-by: Tsz Wai Ko <[email protected]> * fix the united test in test_bond.py * fix the error from the upgrade of boto3 * Downgrade DGL to 2.2.1 * Downgrade pytorch * fix mypy by adding self.norm_layers is not None --------- Signed-off-by: Tsz Wai Ko <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Shyue Ping Ong <[email protected]>
Signed-off-by: Tsz Wai Ko <[email protected]>
Signed-off-by: Tsz Wai Ko <[email protected]>
Signed-off-by: Tsz Wai Ko <[email protected]>
WalkthroughThe pull request introduces several enhancements to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (6)
src/matgl/layers/_so3.py (3)
Line range hint
1-4: Enhance module docstring to follow Google format and include complete attribution.The module docstring should be enhanced to:
- Follow Google format
- Include complete attribution details (version, license)
Consider updating to:
-"""Main components in SO3Net, the implementations are token from Schnetpack2.0 -(https://github.com/atomistic-machine-learning/schnetpack in schnetpack/src/schnetpack/nn/so3.py). +"""SO3Net components for handling 3D rotational operations. + +This module implements SO3-equivariant operations including spherical harmonics, +tensor products, and convolutions. + +Note: + Implementation adapted from Schnetpack 2.0 + (https://github.com/atomistic-machine-learning/schnetpack/blob/master/src/schnetpack/nn/so3.py) + Version: <version> + License: <license> """
Line range hint
279-349: LGTM! Consider enhancing docstring format.The implementation is solid with proper type hints, mathematical documentation, and efficient use of torch buffers. Consider enhancing the docstring to fully follow Google format.
Update the docstring to:
class SO3TensorProduct(nn.Module): - r""" - SO3-equivariant Clebsch-Gordon tensor product. + r"""SO3-equivariant Clebsch-Gordon tensor product. + + This class implements the SO3-equivariant Clebsch-Gordon tensor product operation. + It computes the tensor product of SO3 features using Clebsch-Gordon coefficients. With combined indexing s=(l,m), this can be written as: .. math:: y_{s,f} = \sum_{s_1,s_2} x_{2,s_2,f} x_{1,s_2,f} C_{s_1,s_2}^{s}. + Attributes: + lmax: Maximum angular momentum in spherical harmonics. + idx_in_1: Input indices for first tensor. + idx_in_2: Input indices for second tensor. + idx_out: Output indices. + clebsch_gordan: Clebsch-Gordon coefficients. """
Line range hint
28-278: Consider standardizing docstrings across all classes.While the implementations are solid, consider standardizing all class and method docstrings to follow the Google format consistently. This includes:
- RealSphericalHarmonics
- SO3Convolution
- SO3ParametricGatedNonlinearity
- SO3GatedNonlinearity
The standardization would improve code documentation and satisfy ruff requirements.
Consider using a docstring template across all classes with sections:
- Brief description
- Detailed description (if needed)
- Mathematical formulation (if applicable)
- Attributes
- Examples (if helpful)
Also applies to: 350-551
src/matgl/ext/ase.py (3)
408-409: Remove trailing whitespaceThere's trailing whitespace at the end of line 408.
- ensemble (str): choose from "nve", "nvt", "nvt_langevin", "nvt_andersen", "nvt_bussi", "npt", + ensemble (str): choose from "nve", "nvt", "nvt_langevin", "nvt_andersen", "nvt_bussi", "npt",🧰 Tools
🪛 Ruff (0.7.0)
408-408: Trailing whitespace
Remove trailing whitespace
(W291)
Line range hint
89-91: Avoid mutable default argumentsUsing mutable default arguments in Python can lead to unexpected behavior if the same list object is reused across multiple calls.
def calculate( self, atoms: Atoms | None = None, - properties: list | None = None, - system_changes: list | None = None, + properties: list | None = [], + system_changes: list | None = [], ): """🧰 Tools
🪛 Ruff (0.7.0)
408-408: Trailing whitespace
Remove trailing whitespace
(W291)
407-409: Consider reformatting the ensemble parameter docstringThe long line in the docstring for the ensemble parameter could be reformatted for better readability.
- ensemble (str): choose from "nve", "nvt", "nvt_langevin", "nvt_andersen", "nvt_bussi", "npt", - "npt_berendsen", "npt_nose_hoover" + ensemble (str): choose from: + - "nve": Microcanonical ensemble + - "nvt": Canonical ensemble with Berendsen thermostat + - "nvt_langevin": Canonical ensemble with Langevin thermostat + - "nvt_andersen": Canonical ensemble with Andersen thermostat + - "nvt_bussi": Canonical ensemble with Bussi thermostat + - "npt": Isothermal-isobaric ensemble + - "npt_berendsen": Isothermal-isobaric ensemble with Berendsen thermostat + - "npt_nose_hoover": Isothermal-isobaric ensemble with Nose-Hoover thermostat🧰 Tools
🪛 Ruff (0.7.0)
408-408: Trailing whitespace
Remove trailing whitespace
(W291)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/matgl/ext/ase.py(1 hunks)src/matgl/layers/_so3.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.7.0)
src/matgl/ext/ase.py
408-408: Trailing whitespace
Remove trailing whitespace
(W291)
🔇 Additional comments (1)
src/matgl/ext/ase.py (1)
Line range hint 12-24: LGTM! Well-implemented cell filter selection
The implementation of the ase_cellfilter parameter in the relax method is well-structured with proper type annotations, documentation, and implementation. The addition of params_asecellfilter for fine-tuning the filters is a good design choice.
🧰 Tools
🪛 Ruff (0.7.0)
408-408: Trailing whitespace
Remove trailing whitespace
(W291)
src/matgl/ext/ase.py
Outdated
| ensemble (str): choose from "nve", "nvt", "nvt_langevin", "nvt_andersen", "nvt_bussi", "npt", | ||
| "npt_berendsen", "npt_nose_hoover" |
There was a problem hiding this comment.
💡 Codebase verification
"nvt_bussi" ensemble option is documented but not implemented
The code shows that while "nvt_bussi" is listed in the docstring as a valid ensemble option, there is no corresponding implementation in the if-elif chain. The code will raise a "Ensemble not supported" ValueError when users try to use this ensemble. The option should either be implemented or removed from the docstring.
🔗 Analysis chain
Missing implementation for "nvt_bussi" ensemble
The "nvt_bussi" option has been added to the type annotation but there's no corresponding implementation in the __init__ method's if-elif chain. This will raise a ValueError when users try to use this ensemble.
Let's verify if the ASE package supports Bussi thermostat:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ASE has Bussi thermostat implementation
rg -l "class.*Bussi" || echo "No Bussi thermostat found in the codebase"
# Check if there are any existing tests for Bussi thermostat
rg -l "nvt_bussi" "test_*"
Length of output: 189
Script:
#!/bin/bash
# Let's check the implementation of the ensemble initialization
ast-grep --pattern 'def __init__($$$) {
$$$
if ensemble $$$:
$$$
}'
# Also check for any mentions of Bussi in the codebase
rg -i "bussi" -A 5 -B 5
Length of output: 1007
Script:
#!/bin/bash
# Let's check the actual implementation of the ensemble handling in the initialization
rg "if ensemble ==" src/matgl/ext/ase.py -A 20
Length of output: 48
Script:
#!/bin/bash
# Let's try a different pattern to find the ensemble initialization code
rg "ensemble.*=.*" src/matgl/ext/ase.py -A 30 -B 5
# Also check if there's any other pattern for ensemble handling
rg "elif ensemble" src/matgl/ext/ase.py -A 5
Length of output: 5963
🧰 Tools
🪛 Ruff (0.7.0)
408-408: Trailing whitespace
Remove trailing whitespace
(W291)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #441 +/- ##
=======================================
Coverage 94.82% 94.82%
=======================================
Files 35 35
Lines 3186 3186
=======================================
Hits 3021 3021
Misses 165 165 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Summary
Fix ruff by sorting the class variables in _so3.py
Checklist
ruff.mypy.duecredit@due.dcitedecorators to reference relevant papers by DOI (example)Tip: Install
pre-commithooks to auto-check types and linting before every commit: