support spin #286
support spin #286
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughAdds magnetic-force (mf) metrics and spin-mode support across exploration, task, entrypoint, and training code; introduces new deviation constants, LMP spin task-group wiring, spin-aware training args/behavior, helper for LMP revisions, and accompanying tests and minor formatting edits. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential attention points:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
tests/op/test_run_dp_train.py (1)
657-658: Remove duplicated line.There appears to be a duplicated line that should be removed.
- self.assertDictEqual(jdata, self.expected_init_model_odict_v2_spin)tests/exploration/test_report_trust_levels.py (1)
103-147: Well-structured magnetic force selection test.The test correctly validates that the exploration report handles magnetic force deviation metrics similarly to how it handles force deviation metrics. The test follows the same pattern as existing tests making it consistent and maintainable.
One minor issue: the variable
all_cand_selon line 128 is defined but never used.- all_cand_sel = [(0, 6), (0, 5), (1, 8), (1, 6), (1, 0), (1, 5)]🧰 Tools
🪛 Ruff (0.8.2)
128-128: Local variable
all_cand_selis assigned to but never usedRemove assignment to unused variable
all_cand_sel(F841)
dpgen2/exploration/task/make_task_group_from_config.py (1)
213-218: Avoid using a mutabledictas default argumentUsing a mutable default (
{}) for theArgument("revisions", ...)parameter can lead to unexpected behavior if therevisionsare modified. Consider usingNoneor an immutable fallback to be safe.A possible fix:
-Argument( - "revisions", - dict, - optional=True, - default={}, - doc=doc_revisions, -) +def_revisions_default = {} +Argument( + "revisions", + dict, + optional=True, + default=def_revisions_default, + doc=doc_revisions, +)tests/exploration/test_devi_manager.py (1)
122-139: Remove extraneous 'f' prefixesThe lines at 130 and 136 use 'f' strings without placeholders, triggering lint warnings. Removing the 'f' prefix is recommended.
- f"Error: the number of frames in", + "Error: the number of frames in",🧰 Tools
🪛 Ruff (0.8.2)
130-130: f-string without any placeholders
Remove extraneous
fprefix(F541)
136-136: f-string without any placeholders
Remove extraneous
fprefix(F541)
dpgen2/exploration/task/lmp_spin_task_group.py (2)
2-2: Clean up unused importsThe imports
random,List,lmp_traj_name,plm_output_name, andmake_lmp_inputare unused, which can create confusion and bloat. Consider removing them.-import random ... -from typing import ( - List, - Optional, -) ... -from dpgen2.constants import ( - lmp_traj_name, ... - plm_output_name, -) ... -from .lmp import ( - make_lmp_input, -)Also applies to: 7-7, 14-14, 17-17, 24-24
🧰 Tools
🪛 Ruff (0.8.2)
2-2:
randomimported but unusedRemove unused import:
random(F401)
44-44: Avoid a mutabledictdefault in function signaturesHaving
revisions: dict = {}as a default argument can be risky. Consider setting the default toNone, then initializing it inside the function body to prevent shared state between calls.-def set_lmp( - self, - numb_models: int, - lmp_template_fname: str, - plm_template_fname: Optional[str] = None, - revisions: dict = {}, -) -> None: +def set_lmp( + self, + numb_models: int, + lmp_template_fname: str, + plm_template_fname: Optional[str] = None, + revisions: Optional[dict] = None, +) -> None: + if revisions is None: + revisions = {}🧰 Tools
🪛 Ruff (0.8.2)
44-44: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
dpgen2/exploration/render/traj_render_lammps.py (1)
45-52: Handle potential file reading errors.You've introduced an optional
lammps_input_fileparameter and setself.lammps_inputby reading the contents of the file. Consider catching exceptions (e.g.,FileNotFoundError) or validating the file path to avoid runtime errors if the file is missing or unreadable.+ import os if lammps_input_file is not None: + if not os.path.exists(lammps_input_file): + raise FileNotFoundError(f"{lammps_input_file} does not exist.") self.lammps_input = Path(lammps_input_file).read_text() else: self.lammps_input = Nonetests/exploration/test_lmp_spin_task_group.py (4)
9-10: Clean up unused imports.The following imports are flagged as unused by static analysis:
•typing.List
•typing.Set
•numpy
•exploration.context.dpgen2
•unittest.mock.Mock
•unittest.mock.patch
•dpgen2.constants.plm_input_name
•dpgen2.exploration.task.ExplorationStageConsider removing these to keep the file tidy and avoid confusion.
-from typing import ( - List, - Set, -) -import numpy as np -try: - from exploration.context import ( - dpgen2, - ) -except ModuleNotFoundError: - pass -from unittest.mock import ( - Mock, - patch, -) -from dpgen2.constants import ( - plm_input_name, -) -from dpgen2.exploration.task import ( - ExplorationStage, -)Also applies to: 13-13, 17-17, 23-24, 30-30, 33-33
🧰 Tools
🪛 Ruff (0.8.2)
9-9:
typing.Listimported but unusedRemove unused import
(F401)
10-10:
typing.Setimported but unusedRemove unused import
(F401)
15-21: Use contextlib.suppress for clarity.Instead of an empty
try-except ModuleNotFoundError: pass, consider usingcontextlib.suppressto make the intent clearer and reduce code nesting:-import os -import textwrap -import unittest -from pathlib import ( - Path, -) -from typing import ( - List, - Set, -) -try: - from exploration.context import ( - dpgen2, - ) -except ModuleNotFoundError: - pass +import contextlib +with contextlib.suppress(ModuleNotFoundError): + from exploration.context import dpgen2🧰 Tools
🪛 Ruff (0.8.2)
15-21: Use
contextlib.suppress(ModuleNotFoundError)instead oftry-except-pass(SIM105)
17-17:
exploration.context.dpgen2imported but unused; consider usingimportlib.util.find_specto test for availability(F401)
193-193: Use enumerate for loop index.Replacing manual index tracking with
enumeratecan make the loop more readable and less error-prone:- idx = 0 - for cc, ii, jj, kk in itertools.product(...): - ... - idx += 1 + for idx, (cc, ii, jj, kk) in enumerate(itertools.product(...)): + ...🧰 Tools
🪛 Ruff (0.8.2)
193-193: Use
enumerate()for index variableidxinforloop(SIM113)
218-218: Use enumerate for loop index.Same suggestion here: use
enumerateto handle indexing in your loop.🧰 Tools
🪛 Ruff (0.8.2)
218-218: Use
enumerate()for index variableidxinforloop(SIM113)
dpgen2/exploration/report/report_trust_levels_base.py (1)
247-254: Consider using non-Yoda conditionals for better readabilityThe consistency checks use Yoda conditions (
0 == len(...)) which can be rewritten for better readability.- assert 0 == len(accu & cand) - assert 0 == len(accu & fail) - assert 0 == len(cand & fail) + assert len(accu & cand) == 0 + assert len(accu & fail) == 0 + assert len(accu & fail) == 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
dpgen2/entrypoint/args.py(2 hunks)dpgen2/entrypoint/submit.py(3 hunks)dpgen2/exploration/deviation/deviation_manager.py(3 hunks)dpgen2/exploration/deviation/deviation_std.py(1 hunks)dpgen2/exploration/render/traj_render_lammps.py(3 hunks)dpgen2/exploration/report/report_adaptive_lower.py(13 hunks)dpgen2/exploration/report/report_trust_levels_base.py(9 hunks)dpgen2/exploration/report/report_trust_levels_max.py(1 hunks)dpgen2/exploration/task/__init__.py(1 hunks)dpgen2/exploration/task/lmp_spin_task_group.py(1 hunks)dpgen2/exploration/task/make_task_group_from_config.py(4 hunks)dpgen2/op/run_dp_train.py(3 hunks)tests/exploration/test_devi_manager.py(4 hunks)tests/exploration/test_lmp_spin_task_group.py(1 hunks)tests/exploration/test_report_adaptive_lower.py(2 hunks)tests/exploration/test_report_trust_levels.py(2 hunks)tests/op/test_run_dp_train.py(3 hunks)dpgen2/entrypoint/args.py(1 hunks)dpgen2/entrypoint/submit.py(3 hunks)dpgen2/exploration/render/traj_render_lammps.py(3 hunks)dpgen2/exploration/report/report_adaptive_lower.py(5 hunks)dpgen2/exploration/report/report_trust_levels_base.py(2 hunks)dpgen2/exploration/task/__init__.py(1 hunks)dpgen2/exploration/task/lmp_spin_task_group.py(2 hunks)dpgen2/exploration/task/make_task_group_from_config.py(3 hunks)dpgen2/op/run_dp_train.py(1 hunks)tests/exploration/test_devi_manager.py(2 hunks)tests/exploration/test_lmp_spin_task_group.py(3 hunks)tests/exploration/test_report_adaptive_lower.py(2 hunks)tests/exploration/test_report_trust_levels.py(3 hunks)tests/op/test_run_dp_train.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
dpgen2/exploration/task/__init__.py
17-17: .lmp_template_task_group.LmpTemplateTaskGroup imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
17-17: .lmp_template_task_group.LmpTemplateTaskGroup imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
dpgen2/entrypoint/submit.py
85-85: dpgen2.exploration.task.LmpTemplateTaskGroup imported but unused
Remove unused import
(F401)
85-85: dpgen2.exploration.task.LmpTemplateTaskGroup imported but unused
Remove unused import
(F401)
dpgen2/exploration/report/report_trust_levels_base.py
255-255: Yoda condition detected
Rewrite as len(accu & cand) == 0
(SIM300)
256-256: Yoda condition detected
Rewrite as len(accu & fail) == 0
(SIM300)
257-257: Yoda condition detected
Rewrite as len(cand & fail) == 0
(SIM300)
255-255: Yoda condition detected
Rewrite as len(accu & cand) == 0
(SIM300)
256-256: Yoda condition detected
Rewrite as len(accu & fail) == 0
(SIM300)
257-257: Yoda condition detected
Rewrite as len(cand & fail) == 0
(SIM300)
dpgen2/exploration/task/lmp_spin_task_group.py
44-44: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
2-2: random imported but unused
Remove unused import: random
(F401)
7-7: typing.List imported but unused
Remove unused import: typing.List
(F401)
14-14: dpgen2.constants.lmp_traj_name imported but unused
Remove unused import
(F401)
17-17: dpgen2.constants.plm_output_name imported but unused
Remove unused import
(F401)
24-24: .lmp.make_lmp_input imported but unused
Remove unused import: .lmp.make_lmp_input
(F401)
44-44: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
tests/exploration/test_report_trust_levels.py
128-128: Local variable all_cand_sel is assigned to but never used
Remove assignment to unused variable all_cand_sel
(F841)
tests/exploration/test_lmp_spin_task_group.py
9-9: typing.List imported but unused
Remove unused import
(F401)
10-10: typing.Set imported but unused
Remove unused import
(F401)
13-13: numpy imported but unused
Remove unused import: numpy
(F401)
15-21: Use contextlib.suppress(ModuleNotFoundError) instead of try-except-pass
(SIM105)
17-17: exploration.context.dpgen2 imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
23-23: unittest.mock.Mock imported but unused
Remove unused import
(F401)
24-24: unittest.mock.patch imported but unused
Remove unused import
(F401)
30-30: dpgen2.constants.plm_input_name imported but unused
Remove unused import: dpgen2.constants.plm_input_name
(F401)
33-33: dpgen2.exploration.task.ExplorationStage imported but unused
Remove unused import: dpgen2.exploration.task.ExplorationStage
(F401)
193-193: Use enumerate() for index variable idx in for loop
(SIM113)
218-218: Use enumerate() for index variable idx in for loop
(SIM113)
tests/exploration/test_devi_manager.py
130-130: f-string without any placeholders
Remove extraneous f prefix
(F541)
136-136: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (44)
dpgen2/exploration/task/__init__.py (1)
16-18:Details
✅ Verification successful
Resolve unused import warning.
The import
LmpTemplateTaskGroupis flagged as unused by static analysis. This import appears to be part of the infrastructure needed for the new spin support functionality since the PR mentions "lmp-spin" is similar to existing "lmp-template".
🏁 Script executed:
#!/bin/bash # Check if LmpTemplateTaskGroup is used elsewhere in the codebase rg "LmpTemplateTaskGroup" --type py | grep -v "__init__.py"Length of output: 2890
Review Verified: Retain 'LmpTemplateTaskGroup' Import
Our verification confirms that the
LmpTemplateTaskGroupimport is indeed used (in tests and core modules such asdpgen2/entrypoint/submit.pyand others). Although static analysis flags it as unused within the package files, its usage in the test suite and its role as part of the public API for the new spin support functionality justify keeping the import. If the static analyzer continues to complain, consider adding an inline suppression comment to document its intentional usage.🧰 Tools
🪛 Ruff (0.8.2)
17-17:
.lmp_template_task_group.LmpTemplateTaskGroupimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
dpgen2/op/run_dp_train.py (1)
525-525: Simplified documentation string format.The documentation string for the
spinparameter has been simplified from multi-line to single-line format, which is appropriate for this short description.dpgen2/exploration/deviation/deviation_manager.py (3)
22-24: Good addition of magnetic force deviation metrics.These new class constants for magnetic force deviation (MF) align with the PR objective to include magnetic force as a judgment index. They follow the same naming pattern as existing constants.
38-40: Proper validation for new metrics.The new magnetic force deviation constants are correctly added to the validation check in
_check_namemethod.
73-75: Updated documentation to include new metrics.The docstring for the
getmethod is appropriately updated to include the new magnetic force deviation metrics in the list of valid deviation names.dpgen2/entrypoint/args.py (2)
215-215: Clear documentation for new parameter.The documentation string clearly explains the purpose of the new
lammps_input_fileparameter, indicating it will be passed to dpdata to parse LAMMPS dump files in spin jobs.
263-265: Well-implemented parameter addition.The new parameter is correctly implemented as an optional string argument with proper documentation. This addition aligns with the PR objective to add a LAMMPS input file option to support reading spin information.
dpgen2/exploration/deviation/deviation_std.py (1)
68-70: Good addition of magnetic force deviation constants.The addition of these three constants (MAX_DEVI_MF, MIN_DEVI_MF, AVG_DEVI_MF) to the model_devi_names tuple properly extends the validation checks to support magnetic force deviations in the DeviManagerStd class.
dpgen2/exploration/report/report_trust_levels_max.py (2)
112-115: Good addition of magnetic force links.The addition of level_mf_hi_link and level_mf_lo_link variables correctly follows the existing pattern for creating documentation links.
117-117: Documentation properly updated to include magnetic force model deviation.The return statement has been appropriately extended to include references to magnetic force model deviation thresholds, which aligns with the PR objective of including magnetic force as a judgment index.
tests/op/test_run_dp_train.py (3)
90-102: Good addition of spin configuration.The spin configuration is correctly defined with appropriate parameters, including the essential
init_model_start_pref_fmparameter required for magnetic force loss calculation.
181-209: Well-structured expected output dictionary for spin models.The expected output dictionary for spin models is correctly structured with appropriate loss parameters including
start_pref_frandstart_pref_fminstead of juststart_pref_f.
590-653: Good implementation of spin model test.The test method properly validates the spin training configuration by:
- Setting up a test with spin-specific parameters
- Verifying proper script generation with magnetic force loss parameters
- Confirming correct model initialization with the expected parameters
This test appropriately covers the PR objective of supporting spin in the training configuration.
tests/exploration/test_report_trust_levels.py (1)
39-41: Good addition of magnetic force tests.The new test methods are properly added to the existing test suite, ensuring that both random and max selection strategies are tested with magnetic force deviation.
dpgen2/exploration/task/make_task_group_from_config.py (1)
25-27: Skip the false positive about 'unused import'Although the static analysis hints at removing
LmpTemplateTaskGroup, it is indeed referenced within this file (e.g., for"lmp-template"), so please retain it.dpgen2/entrypoint/submit.py (1)
85-85: Newly introducedLmpSpinTaskGrouplooks goodThis addition is used by
"lmp-spin"in the downstream logic and appears functionally consistent.🧰 Tools
🪛 Ruff (0.8.2)
85-85:
dpgen2.exploration.task.LmpTemplateTaskGroupimported but unusedRemove unused import
(F401)
tests/exploration/test_devi_manager.py (4)
34-35: Confirmed test coverage forMIN_DEVI_MFThese new lines accurately verify that
MIN_DEVI_MFdefaults to[None, None], matching expected behavior.
40-41: Post-clear behavior confirmedVerifying that the list is empty after
clear()is a solid addition to ensure reliability.
77-77: Extended coverage withMAX_DEVI_MFAdding this extra assertion broadens test coverage for the new deviation metric.
88-93: Assertion checks for mismatch scenariosThese additional checks help validate error conditions when
model_devi.getis called with incorrect frames, improving robustness.dpgen2/exploration/render/traj_render_lammps.py (3)
82-86: Looks good for managing MF columns.The conditional check for
dd.shape[1] >= 10properly guards against out-of-bounds access when adding magnetic force deviations tomodel_devi.
130-134: Good handling of the optional input file.Writing the LAMMPS input data to
"lammps_input.in"whenself.lammps_inputis notNonealigns well with the newly introducedlammps_input_filelogic. No concerns here.
141-142: Clear inline documentation for spin usage.The inline comment and updated instantiation of
dpdata.Systemwithinput_file=lammps_input_fileillustrate how the spin data is processed. This is consistent with the new spin job requirements.tests/exploration/test_lmp_spin_task_group.py (1)
1-220: Overall test coverage looks solid for spin tasks.This new test suite thoroughly checks the spin template generation and revision logic, helping ensure correctness for different revision parameters and configurations.
🧰 Tools
🪛 Ruff (0.8.2)
9-9:
typing.Listimported but unusedRemove unused import
(F401)
10-10:
typing.Setimported but unusedRemove unused import
(F401)
13-13:
numpyimported but unusedRemove unused import:
numpy(F401)
15-21: Use
contextlib.suppress(ModuleNotFoundError)instead oftry-except-pass(SIM105)
17-17:
exploration.context.dpgen2imported but unused; consider usingimportlib.util.find_specto test for availability(F401)
23-23:
unittest.mock.Mockimported but unusedRemove unused import
(F401)
24-24:
unittest.mock.patchimported but unusedRemove unused import
(F401)
30-30:
dpgen2.constants.plm_input_nameimported but unusedRemove unused import:
dpgen2.constants.plm_input_name(F401)
33-33:
dpgen2.exploration.task.ExplorationStageimported but unusedRemove unused import:
dpgen2.exploration.task.ExplorationStage(F401)
193-193: Use
enumerate()for index variableidxinforloop(SIM113)
218-218: Use
enumerate()for index variableidxinforloop(SIM113)
tests/exploration/test_report_adaptive_lower.py (2)
299-368: Comprehensive test for MF deviation handling.The
test_mfmethod thoroughly verifies how magnetic force deviations are recorded, with sensible checks for candidate, accurate, and failed sets. The convergence logic is also well-covered.
369-439: Good addition of combined V + MF tests.Testing both virial and magnetic force deviation paths in
test_v_mfensures consistent coverage for multi-deviation scenarios, reflecting real usage patterns.dpgen2/exploration/report/report_adaptive_lower.py (7)
30-31: Well-documented MF parameters.Your docstring expansions and parameter definitions for
level_mf_hi,numb_candi_mf, andrate_candi_mfare clear, ensuring users understand how magnetic force fits into the adaptive-lower logic.Also applies to: 35-41, 64-71
90-90: Initialization logic for MF is consistent.Setting
has_mfand defaulting to maximum float values if MF is not provided mirrors the existing pattern for virial. This maintains internal consistency.Also applies to: 93-95, 102-102, 114-118
128-129: Extended printing format for MF.Appending
mf_loandmf_hito the print tuple and spacing arrays is consistent with how forces and virials are handled.Also applies to: 148-154
268-268: Appropriate references to MF in record method.Including MF in the arrays and adding the new column from
DeviManager.MAX_DEVI_MFensures that the final candidate sets properly account for MF.Also applies to: 272-272, 278-278, 281-283, 286-286
308-308: Clear handling of MF candidates and trust levels.Adjusting
level_mf_loand the logic fornumb_candi_mfparallels force/virial blocks. It's good to see consistent checks for empty sets.Also applies to: 313-315, 323-329, 339-344
345-356: Properly flags frames exceeding MF thresholds.The
_record_one_trajmethod integrates MF checks alongside force and virial, systematically marking frames as failed if any exceed the high threshold.Also applies to: 357-364
425-429: MF convergence logic is correctly integrated.Including
level_mf_loin the_sequence_convmethod ensures that the magnetic force trust levels also factor into the ultimate convergence determination.Also applies to: 442-446
dpgen2/exploration/report/report_trust_levels_base.py (11)
34-35: Good implementation of magnetic force level parametersThe addition of magnetic force level parameters (
level_mf_loandlevel_mf_hi) is well-implemented, with proper initialization and a flag to track if both parameters are set. This aligns with the PR objective to include magnetic force as a judgment index.Also applies to: 42-43, 47-47
67-72: Clean extension of the print formatThe conditional logic for including magnetic force levels in the output follows the same pattern as for virial levels, maintaining code consistency.
89-114: Well-documented parameter additionsThe new magnetic force level parameters are properly documented with clear descriptions and follow the same pattern as existing parameters. The formatting of the argument declarations is consistent with the project style.
141-141: Proper integration of magnetic force deviation dataThe magnetic force model deviation data is correctly retrieved and processed using the same pattern as force and virial deviations, ensuring consistent handling across all deviation types.
Also applies to: 150-152
160-162: Consistent parameter updatesThe additional parameters for the
_record_one_trajmethod call correctly pass the magnetic force indexes, maintaining compatibility with the updated method signature.
207-209: Method signature properly extendedThe
_record_one_trajmethod signature has been updated to include parameters for magnetic force indexes, following the same pattern as existing parameters.
220-223: Good error checking for magnetic force dataThe implementation correctly handles the case when magnetic force data is not available, following the same pattern as for virial data.
229-233: Thorough validation of frame countsThe validation of frame counts for magnetic force data ensures data consistency, mirroring the existing validation for virial data.
242-244: Consistent set operations for magnetic forceThe set operations for magnetic force follow the same pattern as for force and virial, maintaining code consistency.
256-258: Clean integration of magnetic force in set operationsThe set operations to determine accurate, failed, and candidate frames now correctly incorporate magnetic force data alongside force and virial data, ensuring comprehensive evaluation.
🧰 Tools
🪛 Ruff (0.8.2)
256-256: Yoda condition detected
Rewrite as
len(accu & fail) == 0(SIM300)
257-257: Yoda condition detected
Rewrite as
len(cand & fail) == 0(SIM300)
329-334: Consistent output formattingThe conditional inclusion of magnetic force levels in the printed output follows the same pattern as for virial levels, maintaining formatting consistency.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #286 +/- ##
==========================================
+ Coverage 84.17% 84.22% +0.04%
==========================================
Files 104 105 +1
Lines 6111 6276 +165
==========================================
+ Hits 5144 5286 +142
- Misses 967 990 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
let me remove the UT under py3.7 |
|
see #287. please merge with master. |
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Could you also please provide example input script and update the doc accordingly?
| model_devi.add(DeviManager.MIN_DEVI_F, dd[:, 5]) # type: ignore | ||
| model_devi.add(DeviManager.AVG_DEVI_F, dd[:, 6]) # type: ignore | ||
| # assume the 7-9 columns are for MF | ||
| if dd.shape[1] >= 10: # type: ignore |
There was a problem hiding this comment.
It seems to be better to judge from the lammps input if the spin model deviation is expected to be output or not.
you may implement a method like self.has_spin(self.lammps_input).
If the input implies spin model deviation output, then the shape[1] of dd must be 9 otherwise must be 6.
| if self.lammps_input is not None: | ||
| lammps_input_file = "lammps_input.in" | ||
| Path(lammps_input_file).write_text(self.lammps_input) |
There was a problem hiding this comment.
What is the purpose of dumping the lammps input script?
| set_f_cand = set(id_f_cand) | ||
| set_f_fail = set(id_f_fail) | ||
| set_v_accu = set([ii for ii in range(nframes)]) if novirial else set(id_v_accu) | ||
| set_v_accu = set_full if novirial else set(id_v_accu) |
There was a problem hiding this comment.
dangerous to assign set_full to set_v_acc, as set_v_acc will be a reference to set_full, which would introduce some unexpected behavior. see the following code
set_full = set([1,2,3])
set_v = set_full
set_mf = set_full
set_v.add(4)
print(set_mf)The code gives
{1, 2, 3, 4}
| ) | ||
|
|
||
|
|
||
| class LmpSpinTaskGroup(ConfSamplingTaskGroup): |
There was a problem hiding this comment.
It seems that this task group generate lammps spin md tasks from templates.
I would recommend naming it as, e.g. LmpSpinTemplateTaskGroup, and use input name like "lmp-spin-template"
| self.lmp_template = Path(lmp_template_fname).read_text().split("\n") | ||
| self.revisions = revisions | ||
| self.lmp_set = True | ||
| self.model_list = sorted([model_name_pattern % ii for ii in range(numb_models)]) |
There was a problem hiding this comment.
What's the difference between a spin task group and a normal template task group? It seems that the spin task group does not revise input models in the template.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dpgen2/entrypoint/submit.py(3 hunks)dpgen2/op/run_dp_train.py(3 hunks)tests/op/test_run_dp_train.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
dpgen2/entrypoint/submit.py
84-84: dpgen2.exploration.task.LmpSpinTaskGroup imported but unused
Remove unused import
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (3.10)
- GitHub Check: build (3.9)
- GitHub Check: build (3.8)
🔇 Additional comments (10)
dpgen2/op/run_dp_train.py (4)
427-433: Implementation correctly handles force prefactors for spin jobs.The conditional block adds specific handling for spin jobs by using different parameter keys in the loss dictionary. For spin jobs, it uses "start_pref_fr" and "start_pref_fm" instead of "start_pref_f", allowing separate prefactors for regular and magnetic forces.
437-441: Consistent implementation for non-nested loss dictionary.The same conditional logic for spin jobs is correctly implemented in the case of a non-nested loss dictionary, ensuring consistent behavior regardless of the loss dictionary structure.
530-534: Clear documentation for the magnetic force prefactor parameter.The documentation string clearly explains that this parameter is specific to spin jobs, which helps users understand its purpose and usage context.
604-617: Well-defined new spin-related arguments.The implementation adds two new arguments to support spin jobs:
init_model_start_pref_fm: Default value of 100 for the magnetic force prefactorspin: Boolean flag to indicate if the job is a spin jobBoth arguments have appropriate types, defaults, and clear documentation.
dpgen2/entrypoint/submit.py (2)
322-324: Improved constructor formatting with explicit lammps_input_file parameter.The
TrajRenderLammpsconstructor has been reformatted for better readability and now explicitly includes thelammps_input_fileparameter from the configuration.
384-388: Consistent parameter passing across different exploration scheduler functions.Similar to the previous change, the
TrajRenderLammpsconstructor in this function is also reformatted to explicitly include thelammps_input_fileparameter, ensuring consistent parameter handling across different exploration scheduler functions.tests/op/test_run_dp_train.py (4)
92-102: Appropriate spin configuration for testing.A new
config_spindictionary is defined with appropriate parameters for testing spin jobs, including:
"spin": Trueto enable spin mode- All standard parameters from the regular config
- The new magnetic force prefactor parameter
"init_model_start_pref_fm": 100
184-211: Well-defined expected output for spin jobs.The
expected_init_model_odict_v2_spindictionary correctly defines the expected output for spin jobs, using"start_pref_fr"and"start_pref_fm"instead of"start_pref_f"in the loss section, which properly tests the spin-specific parameter handling.
602-607: Good test preparation for spin-specific loss parameters.The test setup creates a modified input dictionary with the appropriate spin-specific loss parameters (
start_pref_frandstart_pref_fm), ensuring the test accurately represents real-world usage.
592-660: Comprehensive test for spin job initialization.The
test_exec_v2_init_model_spinmethod thoroughly tests the spin job functionality by:
- Setting up appropriate spin configuration with init_model_policy = "yes"
- Verifying correct command calls with --init-frz-model flag
- Validating output file existence and log content
- Confirming that the generated training script contains the expected spin-specific parameters
This ensures that the spin job implementation works correctly with model initialization.
| CustomizedLmpTemplateTaskGroup, | ||
| ExplorationStage, | ||
| ExplorationTask, | ||
| LmpSpinTaskGroup, |
There was a problem hiding this comment.
Unused import detected.
The LmpSpinTaskGroup import is not used within this file according to static analysis.
Consider removing this import if it's not needed or add a comment explaining why it's included for future reference.
- LmpSpinTaskGroup,📝 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.
| LmpSpinTaskGroup, |
🧰 Tools
🪛 Ruff (0.11.9)
84-84: dpgen2.exploration.task.LmpSpinTaskGroup imported but unused
Remove unused import
(F401)
🤖 Prompt for AI Agents
In dpgen2/entrypoint/submit.py at line 84, the import LmpSpinTaskGroup is not
used anywhere in the file. Remove this import statement to clean up the code
unless there is a specific reason to keep it, in which case add a comment
explaining its purpose for future maintainers.
| ) | ||
| with open(out["script"]) as fp: | ||
| jdata = json.load(fp) | ||
| print(jdata) |
There was a problem hiding this comment.
Remove debug print statement.
There's a debug print statement that should be removed before merging.
- print(jdata)📝 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.
| print(jdata) |
🤖 Prompt for AI Agents
In tests/op/test_run_dp_train.py at line 658, there is a debug print statement
printing jdata. Remove this print statement to clean up the code before merging.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
dpgen2/entrypoint/submit.py (1)
77-77: Unused import flagged by static analysis.This import remains unused within the file. If it's intended for public API exposure, consider adding an
__all__list or removing the import if not needed.
🧹 Nitpick comments (3)
tests/exploration/test_report_adaptive_lower.py (2)
371-440: Address the TODO comment and clarify test intent.The
#### need modifycomment at line 371 indicates this test is incomplete. Additionally, the test nametest_v_mfsuggests testing virial + magnetic force together, but:
- No
MAX_DEVI_MFdata is added tomodel_devi- No
level_mf_hiparameter is passed toExplorationReportAdaptiveLowerThis effectively tests V-only mode (same as
test_v) with an updatedMockedReportthat includeslevel_mf_lo. Consider either:
- Renaming the test to clarify its purpose (e.g.,
test_v_with_mf_mock_attr)- Or completing the test to actually exercise V+MF candidate selection together
Would you like me to generate a complete
test_v_mfimplementation that adds bothMAX_DEVI_VandMAX_DEVI_MFdeviations and configures both virial and magnetic force parameters?
461-462: Consider adding assertion forrate_candi_mfdefault.For consistency with the existing check for
rate_candi_vat line 457, consider adding an assertion forrate_candi_mf:self.assertTrue(data["level_mf_hi"] is None) self.assertEqual(data["numb_candi_mf"], 0) + self.assertAlmostEqual(data["rate_candi_mf"], 0.0)dpgen2/exploration/task/make_task_group_from_config.py (1)
706-714: Code pattern duplicates the lmp-template branch.This branch is nearly identical to the "lmp-template" branch (lines 697-705). While the current implementation is clear and allows for future divergence, you could optionally refactor to reduce duplication if both task groups continue to follow the same initialization pattern.
If desired, consider a helper function:
def _create_template_task_group(config, numb_models, task_group_class): tgroup = task_group_class() config.pop("type") lmp_template = config.pop("lmp_template_fname") tgroup.set_lmp(numb_models, lmp_template, **config) return tgroupThen use:
tgroup = _create_template_task_group(config, numb_models, LmpSpinTaskGroup)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dpgen2/entrypoint/submit.py(3 hunks)dpgen2/exploration/report/report_adaptive_lower.py(13 hunks)dpgen2/exploration/task/make_task_group_from_config.py(4 hunks)tests/exploration/test_report_adaptive_lower.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use semantic commit messages for all PR titles and commit messages following the format:<type>(<scope>): <description>with optional body and footers. Common types include: feat, fix, docs, style, refactor, test, chore, perf, ci
Use ruff format to format Python code before committing
Use isort to organize and sort imports in Python files before committing
Ensure all changes pass the validation test suite including entrypoint.test_argparse, entrypoint.test_workflow, and test_collect_data before committing
Files:
dpgen2/entrypoint/submit.pydpgen2/exploration/task/make_task_group_from_config.pydpgen2/exploration/report/report_adaptive_lower.pytests/exploration/test_report_adaptive_lower.py
dpgen2/entrypoint/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
CLI command implementations should be located in dpgen2/entrypoint/ and validate arguments properly
Files:
dpgen2/entrypoint/submit.py
tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run unit tests with SKIP_UT_WITH_DFLOW=0 DFLOW_DEBUG=1 environment variables to execute workflows locally with mocked operators
Files:
tests/exploration/test_report_adaptive_lower.py
🧬 Code graph analysis (3)
dpgen2/entrypoint/submit.py (2)
dpgen2/exploration/task/lmp_spin_task_group.py (1)
LmpSpinTaskGroup(31-105)dpgen2/exploration/render/traj_render_lammps.py (1)
TrajRenderLammps(40-152)
dpgen2/exploration/task/make_task_group_from_config.py (1)
dpgen2/exploration/task/lmp_spin_task_group.py (2)
LmpSpinTaskGroup(31-105)set_lmp(39-52)
tests/exploration/test_report_adaptive_lower.py (5)
dpgen2/exploration/deviation/deviation_std.py (1)
DeviManagerStd(17-105)dpgen2/exploration/deviation/deviation_manager.py (2)
add(43-58)DeviManager(13-93)dpgen2/exploration/report/report_adaptive_lower.py (6)
record(278-360)converged(428-446)get_candidate_ids(469-482)candidate_ratio(460-464)accurate_ratio(454-458)failed_ratio(448-452)dpgen2/exploration/report/report_trust_levels_base.py (6)
record(136-185)converged(271-275)get_candidate_ids(299-303)candidate_ratio(289-293)accurate_ratio(283-287)failed_ratio(277-281)dpgen2/exploration/report/report_trust_levels_max.py (2)
converged(28-46)get_candidate_ids(48-61)
🪛 Ruff (0.14.7)
dpgen2/exploration/report/report_adaptive_lower.py
378-381: Avoid specifying long messages outside the exception class
(TRY003)
383-386: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (3)
- GitHub Check: build (3.10)
- GitHub Check: build (3.8)
- GitHub Check: build (3.9)
🔇 Additional comments (9)
tests/exploration/test_report_adaptive_lower.py (1)
301-369: LGTM!The
test_mfmethod properly validates MF-only candidate selection by disabling force candidates (level_f_hi=1.0,numb_candi_f=0) and enabling MF parameters. The test structure mirrors existing tests (test_v) and correctly verifies candidate/accurate/failed sets, convergence behavior, and ratio calculations.dpgen2/exploration/report/report_adaptive_lower.py (6)
93-118: LGTM!The MF parameter handling correctly mirrors the virial pattern: when
level_mf_hiisNone,has_mfis set toFalseand the threshold defaults tosys.float_info.maxto ensure no frames are marked as failed due to MF deviation.
232-244: LGTM!The argument definitions for MF parameters are consistent with the existing F and V patterns, with correct types, defaults, and documentation.
286-360: LGTM!The
record()method correctly integrates MF deviation handling:
- Retrieves
MAX_DEVI_MFfrom the deviation manager- Maintains frame count consistency via assertion at line 316
- Computes
level_mf_loand candidate counts following the established virial pattern
382-414: LGTM!The
_record_one_trajmethod correctly handles MF deviations:
- Validates that MF data is provided when
has_mf=True- Uses zeros fallback when MF is not enabled, ensuring the threshold check always passes
- Integrates MF into the failure condition alongside force and virial
442-445: LGTM!The convergence check correctly extends to MF when enabled, using the same
_sequence_convpattern as force and virial levels.
629-633: LGTM!Print output correctly includes MF levels when enabled, matching the header format defined in the constructor.
dpgen2/exploration/task/make_task_group_from_config.py (2)
22-24: LGTM!The import is properly used in the
make_lmp_task_group_from_configfunction at line 707.
203-239: Consider refactoring to reduce duplication and verify missing parameters.The doc strings for
lmp_template_fnameandplm_template_fname(lines 204-205) appear to be duplicated fromlmp_template_task_group_args(). Additionally, this function omits several arguments present in the template version:traj_freq,extra_pair_style_args,pimd_bead, andinput_extra_files.Verify whether these omissions are intentional for spin-specific tasks by checking the
LmpSpinTaskGroupimplementation. If these parameters are genuinely not needed for spin tasks, the current implementation is acceptable. Otherwise, consider adding them to maintain consistency with the template task group.
| render = TrajRenderLammps( | ||
| nopbc=output_nopbc, lammps_input_file=config["explore"]["lammps_input_file"] | ||
| ) |
There was a problem hiding this comment.
Potential KeyError if configuration key is missing.
Direct dictionary access to config["explore"]["lammps_input_file"] will raise a KeyError if the key doesn't exist. Since lammps_input_file is an optional parameter with default None, use .get() to safely access the configuration key.
Apply this diff:
- render = TrajRenderLammps(
- nopbc=output_nopbc, lammps_input_file=config["explore"]["lammps_input_file"]
- )
+ render = TrajRenderLammps(
+ nopbc=output_nopbc, lammps_input_file=config["explore"].get("lammps_input_file")
+ )📝 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.
| render = TrajRenderLammps( | |
| nopbc=output_nopbc, lammps_input_file=config["explore"]["lammps_input_file"] | |
| ) | |
| render = TrajRenderLammps( | |
| nopbc=output_nopbc, lammps_input_file=config["explore"].get("lammps_input_file") | |
| ) |
🤖 Prompt for AI Agents
In dpgen2/entrypoint/submit.py around lines 315 to 317, the code accesses
config["explore"]["lammps_input_file"] directly which can raise KeyError if
"explore" or "lammps_input_file" is missing; change the access to use safe .get
calls (e.g. config.get("explore", {}).get("lammps_input_file", None)) and pass
that value (which will be None by default) into TrajRenderLammps so the optional
parameter is handled safely.
| render = TrajRenderLammps( | ||
| nopbc=output_nopbc, | ||
| use_ele_temp=use_ele_temp, | ||
| lammps_input_file=config["explore"]["lammps_input_file"], | ||
| ) |
There was a problem hiding this comment.
Potential KeyError if configuration key is missing.
Direct dictionary access to config["explore"]["lammps_input_file"] will raise a KeyError if the key doesn't exist. Since lammps_input_file is an optional parameter with default None, use .get() to safely access the configuration key.
Apply this diff:
- render = TrajRenderLammps(
- nopbc=output_nopbc,
- use_ele_temp=use_ele_temp,
- lammps_input_file=config["explore"]["lammps_input_file"],
- )
+ render = TrajRenderLammps(
+ nopbc=output_nopbc,
+ use_ele_temp=use_ele_temp,
+ lammps_input_file=config["explore"].get("lammps_input_file"),
+ )📝 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.
| render = TrajRenderLammps( | |
| nopbc=output_nopbc, | |
| use_ele_temp=use_ele_temp, | |
| lammps_input_file=config["explore"]["lammps_input_file"], | |
| ) | |
| render = TrajRenderLammps( | |
| nopbc=output_nopbc, | |
| use_ele_temp=use_ele_temp, | |
| lammps_input_file=config["explore"].get("lammps_input_file"), | |
| ) |
🤖 Prompt for AI Agents
In dpgen2/entrypoint/submit.py around lines 377 to 381, the code accesses
config["explore"]["lammps_input_file"] directly which can raise KeyError if the
key is missing; change the access to use .get() (e.g., config.get("explore",
{}).get("lammps_input_file")) so the optional lammps_input_file defaults to None
when absent, and pass that value into TrajRenderLammps.
| virial model deviation of the virial frames. The lower magnetic force trust | ||
| level will be set to the lowest magnetic force model deviation of the magnetic | ||
There was a problem hiding this comment.
Fix truncated docstring.
The sentence is incomplete. It should end with "...of the magnetic force frames." similar to the force and virial descriptions above.
- lower than `level_mf_hi` as candidates.
+ lower than `level_mf_hi` as candidates. The lower magnetic force trust
+ level will be set to the lowest magnetic force model deviation of the
+ magnetic force frames.Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In dpgen2/exploration/report/report_adaptive_lower.py around lines 39 to 41, the
docstring sentence is truncated; complete the sentence by appending "of the
magnetic force frames." so it matches the force and virial descriptions and
reads "...of the magnetic force frames." ensuring the docstring is coherent and
consistent with the other descriptions.
| Argument( | ||
| "lmp-spin", | ||
| dict, | ||
| lmp_spin_task_group_args(), | ||
| doc=doc_lmp_template, | ||
| ), |
There was a problem hiding this comment.
Use spin-specific documentation.
Line 372 reuses doc_lmp_template from the "lmp-template" variant. Consider creating dedicated documentation (e.g., doc_lmp_spin) that specifically describes the spin task group's purpose and any differences from the template version.
Apply this diff to add spin-specific documentation:
+ doc_lmp_spin = "Lammps SPIN MD tasks defined by templates. User provide lammps template for spin simulation tasks. The variables in templates are revised by the revisions key."
return Variant(
"type",
[
Argument(
"lmp-md", dict, npt_task_group_args(), alias=["lmp-npt"], doc=doc_lmp_md
),
Argument(
"lmp-template",
dict,
lmp_template_task_group_args(),
doc=doc_lmp_template,
),
Argument(
"lmp-spin",
dict,
lmp_spin_task_group_args(),
- doc=doc_lmp_template,
+ doc=doc_lmp_spin,
),Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In dpgen2/exploration/task/make_task_group_from_config.py around lines 368 to
373, the Argument for "lmp-spin" reuses doc_lmp_template; create a dedicated
doc_lmp_spin string describing the spin task group's purpose and differences
from the template, then replace the doc_lmp_template reference with doc_lmp_spin
for this Argument; ensure the new doc variable is defined near the other doc
strings and follows the same formatting and localization pattern as
doc_lmp_template.
3 participants
Support the spin job:
For a spin job, most of the contents of the submit config file are same as a normal job, except for:
A lammps template file may be like:
Need replace "SEED" to a random number, the command may be like:"a=$RANDOM && sed -i "s/SEED/$a/g" in.lammps"
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.