Setting the default directory for storing MGLDataset in the current working directory. #503
Conversation
Signed-off-by: Tsz Wai Ko <47970742+kenko911@users.noreply.github.com>
…taining and using element offsets
…ction including both
…the update of sympy
* improve TensorNet model coverage * Update pyproject.toml Signed-off-by: Tsz Wai Ko <47970742+kenko911@users.noreply.github.com> * 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 <47970742+kenko911@users.noreply.github.com> * 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 <47970742+kenko911@users.noreply.github.com> * Converted the float number into tensor Signed-off-by: Tsz Wai Ko <47970742+kenko911@users.noreply.github.com> * 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 <47970742+kenko911@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Shyue Ping Ong <shyuep@users.noreply.github.com>
Signed-off-by: Tsz Wai Ko <47970742+kenko911@users.noreply.github.com>
Signed-off-by: Tsz Wai Ko <47970742+kenko911@users.noreply.github.com>
Signed-off-by: Tsz Wai Ko <47970742+kenko911@users.noreply.github.com>
|
Warning Rate limit exceeded@kenko911 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces several modifications across different files in the matgl library. The changes primarily focus on improving the Changes
Sequence DiagramsequenceDiagram
participant User
participant MGLDataset
participant ASECalculator
User->>MGLDataset: Create dataset with directory_name
MGLDataset-->>User: Dataset initialized with default paths
User->>ASECalculator: Create calculator with use_voigt option
ASECalculator-->>User: Calculator ready with stress tensor handling
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/matgl/ext/ase.py (1)
Line range hint
3-35: Fix import block sorting.The import block needs to be sorted according to the linting rules.
from __future__ import annotations import collections import contextlib import io import pickle import sys from enum import Enum from typing import TYPE_CHECKING, Literal import ase.optimize as opt import numpy as np import pandas as pd import scipy.sparse as sp from ase import Atoms, units -from ase.stress import full_3x3_to_voigt_6_stress from ase.calculators.calculator import Calculator, all_changes from ase.constraints import ExpCellFilter from ase.filters import FrechetCellFilter from ase.md import Langevin from ase.md.andersen import Andersen from ase.md.bussi import Bussi from ase.md.npt import NPT from ase.md.nptberendsen import Inhomogeneous_NPTBerendsen, NPTBerendsen from ase.md.nvtberendsen import NVTBerendsen from ase.md.velocitydistribution import MaxwellBoltzmannDistribution from ase.md.verlet import VelocityVerlet +from ase.stress import full_3x3_to_voigt_6_stress from pymatgen.core.structure import Molecule, Structure from pymatgen.io.ase import AseAtomsAdaptor from pymatgen.optimization.neighbors import find_points_in_spheres
🧹 Nitpick comments (1)
src/matgl/graph/data.py (1)
Line range hint
155-164: Consider enhancing the parameter documentation.While the documentation is accurate, it could be more descriptive:
- For
directory_name: Add that this directory will be created undersave_dirif it doesn't exist- For
raw_dir: Clarify that this is relative to the current working directory- directory_name: name of the generated directory that stores the dataset. + directory_name: Name of the directory that will be created under save_dir to store the dataset. graph_labels: state attributes. clear_processed: Whether to clear the stored structures after processing into graphs. Structures are not really needed after the conversion to DGL graphs and can take a significant amount of memory. Setting this to True will delete the structures from memory. save_cache: whether to save the processed dataset. The dataset can be reloaded from save_dir Default: True - raw_dir : str specifying the directory that will store the downloaded data or the directory that already - stores the input data. - Default: current working directory + raw_dir: Directory path (relative to current working directory) that will store downloaded data + or already contains input data. + Default: "./" (current working directory) save_dir : directory to save the processed dataset. Default: same as raw_dir.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/Training a M3GNet Potential with PyTorch Lightning.ipynb(1 hunks)src/matgl/ext/ase.py(4 hunks)src/matgl/graph/data.py(4 hunks)tests/graph/test_data.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/Training a M3GNet Potential with PyTorch Lightning.ipynb
🧰 Additional context used
🪛 GitHub Actions: Linting
src/matgl/ext/ase.py
[error] 3-35: Import block is un-sorted or un-formatted
[error] 132-132: Missing argument description in the docstring for __init__: use_voigt
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test (3.12, macos-latest)
- GitHub Check: test (3.12, ubuntu-latest)
- GitHub Check: test (3.10, macos-latest)
- GitHub Check: test (3.10, ubuntu-latest)
🔇 Additional comments (9)
src/matgl/graph/data.py (2)
133-138: LGTM! Parameter changes improve clarity and usability.The renaming of
nametodirectory_nameand setting default values forraw_dirandsave_dirto"./"makes the storage location more explicit and aligns with the PR objective of storing datasets in the current working directory.
183-183: LGTM! Consistent parameter usage in super().init.The change correctly passes the renamed parameter to the parent class.
src/matgl/ext/ase.py (3)
18-18: LGTM! Required import for stress tensor conversion.The import of
full_3x3_to_voigt_6_stressis necessary for the new Voigt notation feature.
159-159: LGTM! Proper instance variable initialization.The
use_voigtparameter is correctly stored as an instance variable.
192-197: LGTM! Clean implementation of stress tensor conversion.The conditional conversion to Voigt notation is well-implemented:
- Uses ternary operator for clean code
- Preserves original behavior when
use_voigtis False- Correctly applies the conversion function when
use_voigtis Truetests/graph/test_data.py (4)
75-75: LGTM! Test updated to use new parameter name.The test correctly uses the renamed parameter
directory_name.
88-88: LGTM! Test updated to use new parameter name.The test correctly uses the renamed parameter
directory_name.
130-130: LGTM! Test updated to use new parameter name.The test correctly uses the renamed parameter
directory_name.
151-151: LGTM! Test updated to use new parameter name.The test correctly uses the renamed parameter
directory_name.
| @@ -133,6 +134,7 @@ def __init__( | |||
| potential: Potential, | |||
| state_attr: torch.Tensor | None = None, | |||
| stress_weight: float = 1.0, | |||
| use_voigt: bool = False, | |||
There was a problem hiding this comment.
Add documentation for the use_voigt parameter.
The docstring is missing the description for the use_voigt parameter.
stress_weight (float): conversion factor from GPa to eV/A^3, if it is set to 1.0, the unit is in GPa
+ use_voigt (bool): Whether to convert stress tensor to Voigt notation format.
+ Default: False
**kwargs: Kwargs pass through to super().__init__().Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/matgl/ext/ase.py(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linting
src/matgl/ext/ase.py
[error] 3-35: Import block is un-sorted or un-formatted. This can be fixed automatically using ruff's --fix option.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test (3.12, macos-latest)
- GitHub Check: test (3.12, ubuntu-latest)
- GitHub Check: test (3.10, macos-latest)
- GitHub Check: test (3.10, ubuntu-latest)
🔇 Additional comments (2)
src/matgl/ext/ase.py (2)
137-137: LGTM! Well-documented parameter addition.The
use_voigtparameter is properly added with a default value that maintains backward compatibility.Also applies to: 148-148
193-198: LGTM! Clean implementation of Voigt notation support.The stress calculation changes are well-implemented:
- Efficiently uses conditional conversion
- Maintains backward compatibility
- Properly applies stress weight after conversion
src/matgl/ext/ase.py
Outdated
| @@ -15,6 +15,7 @@ | |||
| import pandas as pd | |||
| import scipy.sparse as sp | |||
| from ase import Atoms, units | |||
| from ase.stress import full_3x3_to_voigt_6_stress | |||
There was a problem hiding this comment.
Fix import block sorting.
The import block needs to be sorted according to the linting error.
-from ase.stress import full_3x3_to_voigt_6_stress
from ase.calculators.calculator import Calculator, all_changes
from ase.constraints import ExpCellFilter
from ase.filters import FrechetCellFilter
+from ase.stress import full_3x3_to_voigt_6_stress📝 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.
| from ase.stress import full_3x3_to_voigt_6_stress | |
| from ase.calculators.calculator import Calculator, all_changes | |
| from ase.constraints import ExpCellFilter | |
| from ase.filters import FrechetCellFilter | |
| from ase.stress import full_3x3_to_voigt_6_stress |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #503 +/- ##
=======================================
Coverage 94.84% 94.85%
=======================================
Files 36 36
Lines 3201 3204 +3
=======================================
+ Hits 3036 3039 +3
Misses 165 165 ☔ View full report in Codecov by Sentry. |
Summary
Setting the default directory for storing MGLDataset in the current working directory so that users can easily spot the location of datasets to avoid confusion. Moreover, the option for predicting stresses in voigt notation is now available so that D3 corrections from torch-dftd can be incorporated efficiently
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: