Add HDF5 support for trajs and model_devis #259
Conversation
Signed-off-by: zjgemi <[email protected]>
Signed-off-by: zjgemi <[email protected]>
Signed-off-by: zjgemi <[email protected]>
Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
…frozen_head in run_lmp and run_relax Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: zjgemi <[email protected]>
Signed-off-by: zjgemi <[email protected]>
WalkthroughWalkthroughThe changes enhance argument handling, data processing capabilities, and flexibility across various modules of the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Additional comments not posted (7)
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
CodeRabbit Configuration File (
|
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (3)
dpgen2/op/__init__.py (1)
42-42: LGTM!The new import statement for
RunRelaxHDF5is consistent with the existing import style in the file.Regarding the unused import warning from Ruff, it's likely a false positive in this case. Importing an entity in
__init__.pyallows it to be accessed directly from the package level, even if it's not used within the__init__.pyfile itself.If desired, you can resolve the warning by adding
RunRelaxHDF5to the__all__list to explicitly mark it as part of the public interface:__all__ = [ ..., "RunRelaxHDF5", ]However, this is not strictly necessary if the project doesn't define
__all__for other entities.Tools
Ruff
42-42:
.run_relax.RunRelaxHDF5imported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
dpgen2/exploration/render/traj_render_lammps.py (1)
55-58: Consider simplifying theif-elseblock using a ternary operator.The static analysis tool suggests using a ternary operator instead of the
if-elseblock. This can simplify the code without changing its behavior.Apply this diff to simplify the code:
-if isinstance(fname, HDF5Dataset): - dd = fname.get_data() -else: - dd = np.loadtxt(fname) +dd = fname.get_data() if isinstance(fname, HDF5Dataset) else np.loadtxt(fname)Tools
Ruff
55-58: Use ternary operator
dd = fname.get_data() if isinstance(fname, HDF5Dataset) else np.loadtxt(fname)instead ofif-else-blockReplace
if-else-block withdd = fname.get_data() if isinstance(fname, HDF5Dataset) else np.loadtxt(fname)(SIM108)
dpgen2/op/run_lmp.py (1)
296-318: LGTM with a nitpick!The
freeze_modelfunction implementation looks good.Improve the error message.
Consider providing more context in the error message to help with debugging.
Apply this diff to improve the error message:
def freeze_model(input_model, frozen_model, head=None): freeze_args = "-o %s" % frozen_model if head is not None: freeze_args += " --head %s" % head freeze_cmd = "dp --pt freeze -c %s %s" % (input_model, freeze_args) ret, out, err = run_command(freeze_cmd, shell=True) if ret != 0: logging.error( "".join( ( "freeze failed\n", - "command was", + "command was: ", freeze_cmd, - "out msg", + "\nout msg: ", out, "\n", - "err msg", + "err msg: ", err, "\n", ) ) ) - raise TransientError("freeze failed") + raise TransientError(f"Failed to freeze model {input_model} with command: {freeze_cmd}")
dpgen2/op/run_relax.py
Outdated
| @@ -1,3 +1,4 @@ | |||
| import logging | |||
There was a problem hiding this comment.
Remove unused import.
The logging module is imported but not used in the code. Please remove it.
-import loggingCommittable 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.
| import logging |
Tools
Ruff
1-1:
loggingimported but unusedRemove unused import:
logging(F401)
dpgen2/op/run_relax.py
Outdated
| @staticmethod | ||
| def normalize_config(data={}): | ||
| ta = RunRelax.relax_args() | ||
| base = Argument("base", dict, ta) | ||
| data = base.normalize_value(data, trim_pattern="_*") | ||
| base.check_value(data, strict=False) | ||
| return data |
There was a problem hiding this comment.
Replace the mutable default argument with None.
Using a mutable default argument can lead to unexpected behavior. Please replace it with None and initialize it within the function.
-def normalize_config(data={}):
+def normalize_config(data=None):
+ if data is None:
+ data = {}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.
| @staticmethod | |
| def normalize_config(data={}): | |
| ta = RunRelax.relax_args() | |
| base = Argument("base", dict, ta) | |
| data = base.normalize_value(data, trim_pattern="_*") | |
| base.check_value(data, strict=False) | |
| return data | |
| @staticmethod | |
| def normalize_config(data=None): | |
| if data is None: | |
| data = {} | |
| ta = RunRelax.relax_args() | |
| base = Argument("base", dict, ta) | |
| data = base.normalize_value(data, trim_pattern="_*") | |
| base.check_value(data, strict=False) | |
| return data |
Tools
Ruff
223-223: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
dpgen2/op/run_lmp.py
Outdated
| Argument( | ||
| "model_frozen_head", str, optional=True, default=None, doc=doc_head | ||
| ), |
There was a problem hiding this comment.
Update the argument documentation.
The argument has been renamed from head to model_frozen_head, but the documentation still refers to the old name.
Apply this diff to update the documentation:
-doc_head = "Select a head from multitask"
+doc_model_frozen_head = "Select a head from the multitask model to freeze"
return [
Argument("command", str, optional=True, default="lmp", doc=doc_lmp_cmd),
Argument(
"teacher_model_path",
[BinaryFileInput, str],
optional=True,
default=None,
doc=doc_teacher_model,
),
Argument(
"shuffle_models",
bool,
optional=True,
default=False,
doc=doc_shuffle_models,
),
Argument(
- "model_frozen_head", str, optional=True, default=None, doc=doc_head
+ "model_frozen_head", str, optional=True, default=None, doc=doc_model_frozen_head
),
]Committable suggestion was skipped due to low confidence.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #259 +/- ##
==========================================
- Coverage 83.70% 83.65% -0.05%
==========================================
Files 104 104
Lines 5958 5990 +32
==========================================
+ Hits 4987 5011 +24
- Misses 971 979 +8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
dpgen2/exploration/render/traj_render_lammps.py (1)
55-58: Consider using a ternary operator for conciseness.The if-else block can be replaced with a ternary operator to make the code more concise without changing the functionality.
Apply this diff to refactor the code:
-if isinstance(fname, HDF5Dataset): - dd = fname.get_data() -else: - dd = np.loadtxt(fname) +dd = fname.get_data() if isinstance(fname, HDF5Dataset) else np.loadtxt(fname)Tools
Ruff
55-58: Use ternary operator
dd = fname.get_data() if isinstance(fname, HDF5Dataset) else np.loadtxt(fname)instead ofif-else-blockReplace
if-else-block withdd = fname.get_data() if isinstance(fname, HDF5Dataset) else np.loadtxt(fname)(SIM108)
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
dpgen2/exploration/render/traj_render_lammps.py (1)
62-65: LGTM! Consider using a ternary operator for conciseness.The change to the logic for loading data allows the
_load_one_model_devimethod to handle HDF5 datasets directly, improving its capability to work with different data formats.As suggested by Ruff, consider replacing the
if-elseblock with a ternary operator for conciseness:-if isinstance(fname, HDF5Dataset): - dd = fname.get_data() -else: - dd = np.loadtxt(fname) +dd = fname.get_data() if isinstance(fname, HDF5Dataset) else np.loadtxt(fname)Tools
Ruff
62-65: Use ternary operator
dd = fname.get_data() if isinstance(fname, HDF5Dataset) else np.loadtxt(fname)instead ofif-else-blockReplace
if-else-block withdd = fname.get_data() if isinstance(fname, HDF5Dataset) else np.loadtxt(fname)(SIM108)
| @@ -10,9 +10,13 @@ | |||
| Optional, | |||
| Set, | |||
| Tuple, | |||
| Union, | |||
| ) | |||
|
|
|||
| import dpdata | |||
There was a problem hiding this comment.
Remove unused import.
The dpdata module is imported but unused.
Apply this diff to remove the unused import:
-import dpdataCommittable 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.
| import dpdata |
Tools
Ruff
16-16:
dpdataimported but unusedRemove unused import:
dpdata(F401)
| import dpdata | ||
| import numpy as np |
There was a problem hiding this comment.
Remove unused imports.
The static analysis tool suggests that the following imports are unused:
dpdata(line 15)numpy(line 16)
Please remove these unused imports to keep the code clean.
Tools
Ruff
15-15:
dpdataimported but unusedRemove unused import:
dpdata(F401)
16-16:
numpyimported but unusedRemove unused import:
numpy(F401)
There was a problem hiding this comment.
It seems ruff hasn't been enabled in the dpgen2
There was a problem hiding this comment.
@njzjz, you're correct that Ruff hasn't been enabled in the dpgen2 repository. If you want to enforce linting rules like removing unused imports, you might consider enabling Ruff in the future. For now, you can disregard the suggestion to remove the unused imports.
Sure. |
I realize that for On the other hand, in the HDF5 mode, users cannot conveniently preview file content in UI. That's why HDF5 mode is not employed by default unless performance bottleneck is met. |
Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
Summary by CodeRabbit
New Features
Bug Fixes
Documentation