Add DiffCSP as a new exploration engine #251
Conversation
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]>
for more information, see https://pre-commit.ci
Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
WalkthroughWalkthroughThe recent modifications enhance the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional context usedRuff
Additional comments not posted (2)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Signed-off-by: zjgemi <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Outside diff range, codebase verification and nitpick comments (7)
dpgen2/exploration/task/diffcsp_task_group.py (1)
47-52: Attributes Missing inExplorationTaskClassThe attributes
trj_freq,fmax,steps, andtimeoutare not defined in theExplorationTaskclass. Consider adding these attributes to the class definition to ensure proper functionality.
- File:
dpgen2/exploration/task/task.pyAnalysis chain
Verify attributes of
ExplorationTask.The attributes
trj_freq,fmax,steps, andtimeoutare being assigned toExplorationTask. Verify that these attributes exist inExplorationTaskor modify the class to include them.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the attributes of `ExplorationTask` to ensure they exist. # Test: Search for the `ExplorationTask` class and check for the attributes. ast-grep --lang python --pattern $'class ExplorationTask { $$$ trj_freq: $_ fmax: $_ steps: $_ timeout: $_ $$$ }'Length of output: 638
Script:
#!/bin/bash # Description: Verify the attributes of `ExplorationTask` to ensure they exist. # Search for the definition of the `ExplorationTask` class and print surrounding lines to check for attributes. rg -A 10 'class ExplorationTask' --glob '*.py'Length of output: 1429
Script:
#!/bin/bash # Description: Check for specific attributes in the `ExplorationTask` class. # Search for the attributes `trj_freq`, `fmax`, `steps`, and `timeout` in the `dpgen2/exploration/task/task.py` file. rg 'trj_freq|fmax|steps|timeout' dpgen2/exploration/task/task.pyLength of output: 66
Script:
#!/bin/bash # Description: Check for inheritance or dynamic assignments related to `ExplorationTask`. # Search for the `ExplorationTask` class definition and any inheritance. rg 'class ExplorationTask' --glob '*.py' -A 5 # Search for dynamic assignments to `ExplorationTask` attributes in the codebase. rg 'ExplorationTask' --glob '*.py' -A 5Length of output: 56930
Tools
GitHub Check: pyright
[failure] 49-49:
Cannot assign member "trj_freq" for type "ExplorationTask"
Member "trj_freq" is unknown (reportGeneralTypeIssues)
[failure] 50-50:
Cannot assign member "fmax" for type "ExplorationTask"
Member "fmax" is unknown (reportGeneralTypeIssues)
[failure] 51-51:
Cannot assign member "steps" for type "ExplorationTask"
Member "steps" is unknown (reportGeneralTypeIssues)
[failure] 52-52:
Cannot assign member "timeout" for type "ExplorationTask"
Member "timeout" is unknown (reportGeneralTypeIssues)dpgen2/op/diffcsp_gen.py (2)
1-7: Optimize imports for better readability.Consider grouping imports from the same module together to enhance readability. For instance,
PathandListcan be imported on a single line.from pathlib import Path from typing import List
27-29: Consider usingos.makedirswithexist_ok=True.Instead of checking for the existence of a directory and then creating it, you can use
os.makedirs(output_dir, exist_ok=True)to simplify the code.- if not os.path.exists(output_dir): - os.makedirs(output_dir) + os.makedirs(output_dir, exist_ok=True)tests/exploration/test_distance_conf_filter.py (1)
57-62: Ensure cleanup insetUpmethod.If the
setUpmethod is interrupted, files might not be cleaned up. Consider usingtry-finallyoraddCleanupto ensure cleanup.def setUp(self): try: with open("POSCAR_1", "w") as f: f.write(POSCAR_1) with open("POSCAR_2", "w") as f: f.write(POSCAR_2) finally: self.addCleanup(self.tearDown)dpgen2/exploration/selector/distance_conf_filter.py (3)
1-16: Organize imports for clarity.Consider grouping related imports together and removing any unnecessary ones. The
dargsimport is used, but you might want to ensure all imported modules are necessary.from copy import deepcopy from typing import List import numpy as np import dargs from dargs import Argument from . import ConfFilter
133-140: Initialize class attributes with type hints.Consider adding type hints for class attributes in
DistanceConfFilterto improve code readability and maintainability.class DistanceConfFilter(ConfFilter): def __init__( self, custom_safe_dist: dict = None, safe_dist_ratio: float = 1.0, theta: float = 60.0, length_ratio: float = 5.0 ):
211-254: Ensure documentation consistency inargsmethod.The documentation strings for the
argsmethod should be consistent with the rest of the codebase, ensuring clarity and completeness.- r"""The argument definition of the `ConfFilter`. + r"""The argument definition of the `DistanceConfFilter`.
| from .prep_run_diffcsp import ( | ||
| PrepRunDiffCSP, | ||
| ) |
There was a problem hiding this comment.
Consider using or exporting PrepRunDiffCSP.
The PrepRunDiffCSP class is imported but not used within this module. Consider using it, adding it to __all__, or removing the import if it's unnecessary.
- from .prep_run_diffcsp import (
- PrepRunDiffCSP,
- )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 .prep_run_diffcsp import ( | |
| PrepRunDiffCSP, | |
| ) |
Tools
Ruff
8-8:
.prep_run_diffcsp.PrepRunDiffCSPimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
| from .diffcsp_gen import ( | ||
| DiffCSPGen, | ||
| ) |
There was a problem hiding this comment.
Consider using or exporting DiffCSPGen.
The DiffCSPGen class is imported but not used within this module. Consider using it, adding it to __all__, or removing the import if it's unnecessary.
- from .diffcsp_gen import (
- DiffCSPGen,
- )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 .diffcsp_gen import ( | |
| DiffCSPGen, | |
| ) |
Tools
Ruff
8-8:
.diffcsp_gen.DiffCSPGenimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
| from .prep_relax import ( | ||
| PrepRelax, | ||
| ) |
There was a problem hiding this comment.
Consider using or exporting PrepRelax.
The PrepRelax class is imported but not used within this module. Consider using it, adding it to __all__, or removing the import if it's unnecessary.
- from .prep_relax import (
- PrepRelax,
- )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 .prep_relax import ( | |
| PrepRelax, | |
| ) |
Tools
Ruff
26-26:
.prep_relax.PrepRelaximported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
| from .run_relax import ( | ||
| RunRelax, | ||
| ) |
There was a problem hiding this comment.
Consider using or exporting RunRelax.
The RunRelax class is imported but not used within this module. Consider using it, adding it to __all__, or removing the import if it's unnecessary.
- from .run_relax import (
- RunRelax,
- )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 .run_relax import ( | |
| RunRelax, | |
| ) |
Tools
Ruff
41-41:
.run_relax.RunRelaximported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
| from .lmp_template_task_group import ( | ||
| LmpTemplateTaskGroup, | ||
| ) | ||
| from .make_task_group_from_config import ( | ||
| caly_normalize, | ||
| caly_task_group_args, | ||
| diffcsp_normalize, |
There was a problem hiding this comment.
Consider using or exporting diffcsp_normalize.
The diffcsp_normalize function is imported but not used within this module. Consider using it, adding it to __all__, or removing the import if it's unnecessary.
- from .make_task_group_from_config import (
- diffcsp_normalize,
- )Committable suggestion was skipped due to low confidence.
Tools
Ruff
19-19:
.make_task_group_from_config.diffcsp_normalizeimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
| OPTemplate, | ||
| OutputArtifact, | ||
| OutputParameter, | ||
| Outputs, | ||
| Step, | ||
| Steps, | ||
| Workflow, | ||
| argo_len, | ||
| argo_range, | ||
| argo_sequence, | ||
| download_artifact, | ||
| upload_artifact, |
There was a problem hiding this comment.
Remove unused imports from dflow.
These imports are not used in the file and can be removed to clean up the code.
- from dflow import (
- OPTemplate,
- OutputParameter,
- Workflow,
- argo_len,
- argo_range,
- download_artifact,
- upload_artifact,
- )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.
| OPTemplate, | |
| OutputArtifact, | |
| OutputParameter, | |
| Outputs, | |
| Step, | |
| Steps, | |
| Workflow, | |
| argo_len, | |
| argo_range, | |
| argo_sequence, | |
| download_artifact, | |
| upload_artifact, | |
| OutputArtifact, | |
| Outputs, | |
| Step, | |
| Steps, | |
| argo_sequence, |
Tools
Ruff
21-21:
dflow.OPTemplateimported but unusedRemove unused import
(F401)
23-23:
dflow.OutputParameterimported but unusedRemove unused import
(F401)
27-27:
dflow.Workflowimported but unusedRemove unused import
(F401)
28-28:
dflow.argo_lenimported but unusedRemove unused import
(F401)
29-29:
dflow.argo_rangeimported but unusedRemove unused import
(F401)
31-31:
dflow.download_artifactimported but unusedRemove unused import
(F401)
32-32:
dflow.upload_artifactimported but unusedRemove unused import
(F401)
| safe_dist_dict = { | ||
| "H": 1.2255, | ||
| "He": 0.936, | ||
| "Li": 1.8, | ||
| "Be": 1.56, | ||
| "B": 1.32, | ||
| "C": 1.32, | ||
| "N": 1.32, | ||
| "O": 1.32, | ||
| "F": 1.26, | ||
| "Ne": 1.92, | ||
| "Na": 1.595, | ||
| "Mg": 1.87, | ||
| "Al": 1.87, | ||
| "Si": 1.76, | ||
| "P": 1.65, | ||
| "S": 1.65, | ||
| "Cl": 1.65, | ||
| "Ar": 2.09, | ||
| "K": 2.3, | ||
| "Ca": 2.3, | ||
| "Sc": 2.0, | ||
| "Ti": 2.0, | ||
| "V": 2.0, | ||
| "Cr": 1.9, | ||
| "Mn": 1.95, | ||
| "Fe": 1.9, | ||
| "Co": 1.9, | ||
| "Ni": 1.9, | ||
| "Cu": 1.9, | ||
| "Zn": 1.9, | ||
| "Ga": 2.0, | ||
| "Ge": 2.0, | ||
| "As": 2.0, | ||
| "Se": 2.1, | ||
| "Br": 2.1, | ||
| "Kr": 2.3, | ||
| "Rb": 2.5, | ||
| "Sr": 2.5, | ||
| "Y": 2.1, | ||
| "Zr": 2.1, | ||
| "Nb": 2.1, | ||
| "Mo": 2.1, | ||
| "Tc": 2.1, | ||
| "Ru": 2.1, | ||
| "Rh": 2.1, | ||
| "Pd": 2.1, | ||
| "Ag": 2.1, | ||
| "Cd": 2.1, | ||
| "In": 2.0, | ||
| "Sn": 2.0, | ||
| "Sb": 2.0, | ||
| "Te": 2.0, | ||
| "I": 2.0, | ||
| "Xe": 2.0, | ||
| "Cs": 2.5, | ||
| "Ba": 2.8, | ||
| "La": 2.5, | ||
| "Ce": 2.55, | ||
| "Pr": 2.7, | ||
| "Nd": 2.8, | ||
| "Pm": 2.8, | ||
| "Sm": 2.8, | ||
| "Eu": 2.8, | ||
| "Gd": 2.8, | ||
| "Tb": 2.8, | ||
| "Dy": 2.8, | ||
| "Ho": 2.8, | ||
| "Er": 2.6, | ||
| "Tm": 2.8, | ||
| "Yb": 2.8, | ||
| "Lu": 2.8, | ||
| "Hf": 2.4, | ||
| "Ta": 2.5, | ||
| "W": 2.3, | ||
| "Re": 2.3, | ||
| "Os": 2.3, | ||
| "Ir": 2.3, | ||
| "Pt": 2.3, | ||
| "Au": 2.3, | ||
| "Hg": 2.3, | ||
| "Tl": 2.3, | ||
| "Pb": 2.3, | ||
| "Bi": 2.3, | ||
| "Po": 2.3, | ||
| "At": 2.3, | ||
| "Rn": 2.3, | ||
| "Fr": 2.9, | ||
| "Ra": 2.9, | ||
| "Ac": 2.9, | ||
| "Th": 2.8, | ||
| "Pa": 2.8, | ||
| "U": 2.8, | ||
| "Np": 2.8, | ||
| "Pu": 2.8, | ||
| "Am": 2.8, | ||
| "Cm": 2.8, | ||
| "Cf": 2.3, | ||
| } |
There was a problem hiding this comment.
Consider externalizing safe_dist_dict.
The safe_dist_dict is quite large and could be externalized to a configuration file or a separate module to improve readability and maintainability.
# Example of externalizing the dictionary
# safe_dist_dict.py
safe_dist_dict = {
"H": 1.2255,
"He": 0.936,
# ... rest of the elements
}
# In your main file
from .safe_dist_dict import safe_dist_dict| def check( | ||
| self, | ||
| coords: np.ndarray, | ||
| cell: np.ndarray, | ||
| atom_types: np.ndarray, | ||
| nopbc: bool, | ||
| ): | ||
| from ase import ( | ||
| Atoms, | ||
| ) | ||
| from ase.build import ( | ||
| make_supercell, | ||
| ) | ||
|
|
||
| safe_dist = deepcopy(safe_dist_dict) | ||
| safe_dist.update(self.custom_safe_dist) | ||
| for k in safe_dist: | ||
| # bohr -> ang and multiply by a relaxation ratio | ||
| safe_dist[k] *= 0.529 / 1.2 * self.safe_dist_ratio | ||
|
|
||
| atom_names = list(safe_dist) | ||
| structure = Atoms( | ||
| positions=coords, | ||
| numbers=[atom_names.index(n) + 1 for n in atom_types], | ||
| cell=cell, | ||
| pbc=(not nopbc), | ||
| ) | ||
|
|
||
| cell, _ = structure.get_cell().standard_form() # type: ignore | ||
|
|
||
| if ( | ||
| cell[1][0] > np.tan(self.theta / 180.0 * np.pi) * cell[1][1] | ||
| or cell[2][0] > np.tan(self.theta / 180.0 * np.pi) * cell[2][2] | ||
| or cell[2][1] > np.tan(self.theta / 180.0 * np.pi) * cell[2][2] | ||
| ): | ||
| print("Inclined box") | ||
| return False | ||
|
|
||
| a = cell[0][0] | ||
| b = cell[1][1] | ||
| c = cell[2][2] | ||
|
|
||
| if check_multiples(a, b, c, self.length_ratio): | ||
| print("One side is %s larger than another" % self.length_ratio) | ||
| return False | ||
|
|
||
| P = [[2, 0, 0], [0, 2, 0], [0, 0, 2]] | ||
| extended_structure = make_supercell(structure, P) | ||
|
|
||
| coords = extended_structure.positions | ||
| symbols = extended_structure.get_chemical_symbols() | ||
|
|
||
| num_atoms = len(coords) | ||
| for i in range(num_atoms): | ||
| for j in range(i + 1, num_atoms): | ||
| dist = extended_structure.get_distance(i, j, mic=True) | ||
| type_i = symbols[i] | ||
| type_j = symbols[j] | ||
| dr = safe_dist[type_i] + safe_dist[type_j] | ||
|
|
||
| if dist < dr: | ||
| print( | ||
| f"Dangerous close for {type_i} - {type_j}, {dist:.5f} less than {dr:.5f}" | ||
| ) | ||
| return False | ||
|
|
||
| print("Valid structure") | ||
| return True |
There was a problem hiding this comment.
Improve error handling and logging in check method.
The check method uses print statements for logging. Consider using a logging framework for better control over log levels and outputs.
import logging
# Configure logging at the start of your module
logging.basicConfig(level=logging.INFO)
# Replace print statements with logging
logging.info("Inclined box")
logging.info("One side is %s larger than another", self.length_ratio)
logging.info("Dangerous close for %s - %s, %.5f less than %.5f", type_i, type_j, dist, dr)
logging.info("Valid structure")| def check_multiples(a, b, c, multiple): | ||
| values = [a, b, c] | ||
|
|
||
| for i in range(len(values)): | ||
| for j in range(len(values)): | ||
| if i != j: | ||
| if values[i] > multiple * values[j]: | ||
| print( | ||
| f"Value {values[i]} is {multiple} times greater than {values[j]}" | ||
| ) | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Combine nested if statements.
The nested if statements in check_multiples can be combined for simplicity.
- if i != j:
- if values[i] > multiple * values[j]:
+ if i != j and values[i] > multiple * values[j]: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.
| def check_multiples(a, b, c, multiple): | |
| values = [a, b, c] | |
| for i in range(len(values)): | |
| for j in range(len(values)): | |
| if i != j: | |
| if values[i] > multiple * values[j]: | |
| print( | |
| f"Value {values[i]} is {multiple} times greater than {values[j]}" | |
| ) | |
| return True | |
| return False | |
| def check_multiples(a, b, c, multiple): | |
| values = [a, b, c] | |
| for i in range(len(values)): | |
| for j in range(len(values)): | |
| if i != j and values[i] > multiple * values[j]: | |
| print( | |
| f"Value {values[i]} is {multiple} times greater than {values[j]}" | |
| ) | |
| return True | |
| return False |
Tools
Ruff
124-125: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
dpgen2/entrypoint/args.py
Outdated
| def variant_filter(): | ||
| doc = "the type of the configuration filter." | ||
| var_list = [] | ||
| for kk in conf_filter_styles.keys(): |
There was a problem hiding this comment.
Simplify dictionary key iteration.
Use for kk in conf_filter_styles: instead of for kk in conf_filter_styles.keys(): for better readability and performance.
- for kk in conf_filter_styles.keys():
+ for kk in conf_filter_styles: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.
| for kk in conf_filter_styles.keys(): | |
| for kk in conf_filter_styles: |
Tools
Ruff
183-183: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
zjgemi
changed the title
Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
dpgen2/op/run_relax.py (3)
118-118: Fix type issue withase.calculators.The
SinglePointCalculatoris not part of theasepackage. It appears to be a custom class defined intests/op/test_run_relax.py. Ensure that you are using the correct module or class indpgen2/op/run_relax.py.
dpgen2/op/run_relax.py: Verify and correct the usage ofSinglePointCalculator.Analysis chain
Fix type issue with
ase.calculators.The module
ase.calculatorsdoes not have a membersinglepoint. Ensure that the correct module or class is being used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct import path for `SinglePointCalculator`. # Test: Search for `SinglePointCalculator` in the `ase` package. rg --type python 'SinglePointCalculator'Length of output: 71
Script:
#!/bin/bash # Description: Verify the correct import path for `SinglePointCalculator`. # Test: Search for `SinglePointCalculator` in the `ase` package. rg 'SinglePointCalculator'Length of output: 276
Tools
GitHub Check: pyright
[failure] 118-118:
"calculators" is not a known member of module "ase" (reportGeneralTypeIssues)
142-142: Potential Issue withdpInitializationThe
dpvariable is initialized from thegraphslist, which includes aNoneelement at the first position. Ifjis 0,dpwill beNone, leading to aNoneTypeerror when callingeval(). Ensure thatjis never 0 when accessinggraphs[j]fordp.
- File:
dpgen2/op/run_relax.py- Line: 142
Analysis chain
Fix type issue with
dp.eval().The method
evalis being called on aNoneType. Ensure thatdpis correctly initialized.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify initialization of `dp` and usage of `eval`. # Test: Search for `eval` method in the codebase. rg --type python 'def eval'Length of output: 60
Script:
#!/bin/bash # Description: Verify initialization of `dp` and usage of `eval`. # Search for the initialization of `dp`. rg --type py 'dp\s*=' -A 5 # Search for the `eval` method definition. ast-grep --lang python --pattern 'def eval($_$$$)'Length of output: 3274
Script:
#!/bin/bash # Description: Investigate the `graphs` variable to understand its type and ensure `dp` is not `None`. # Search for the definition or initialization of `graphs`. rg --type py 'graphs\s*=' -A 5Length of output: 1352
Tools
GitHub Check: pyright
[failure] 142-142:
Cannot access member "eval" for type "None"
Member "eval" is unknown (reportGeneralTypeIssues)
139-139: Fix type issue withdp.get_type_map(). The initialization ofgraphswithNoneas its first element can lead todpbeingNoneif accessed with an index of 0. Ensure thatjis never 0 when accessinggraphs[j]to avoid callingget_type_mapon aNoneType.
dpgen2/op/run_relax.py: Ensurejis not 0 when accessinggraphs[j].Analysis chain
Fix type issue with
dp.get_type_map().The method
get_type_mapis being called on aNoneType. Ensure thatdpis correctly initialized.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify initialization of `dp` and usage of `get_type_map`. # Test: Search for `get_type_map` method in the codebase. rg --type python 'def get_type_map'Length of output: 68
Script:
#!/bin/bash # Corrected script to search for `get_type_map` method in Python files. # Find all Python files and search for the definition of `get_type_map`. rg 'def get_type_map' --glob '*.py' -A 5 # Search for the initialization of `dp` in the relevant file. rg 'dp\s*=' --glob '*.py' -A 3Length of output: 2221
Script:
#!/bin/bash # Search for the initialization and usage of `graphs` in `dpgen2/op/run_relax.py`. # Find where `graphs` is initialized or assigned. rg 'graphs\s*=' --glob 'dpgen2/op/run_relax.py' -A 3 # Search for any function or method definitions in the file to understand the context better. rg 'def ' --glob 'dpgen2/op/run_relax.py' -A 2Length of output: 808
Tools
GitHub Check: pyright
[failure] 139-139:
Cannot access member "get_type_map" for type "None"
Member "get_type_map" is unknown (reportGeneralTypeIssues)
Signed-off-by: zjgemi <[email protected]>
Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #251 +/- ##
==========================================
+ Coverage 83.87% 84.05% +0.17%
==========================================
Files 98 103 +5
Lines 5433 5725 +292
==========================================
+ Hits 4557 4812 +255
- Misses 876 913 +37 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
zjgemi
changed the title
There was a problem hiding this comment.
I would recommend cherry-picking the implementation of diffcsp, so this pr is decoupled from #250
… BoxSkewnessConfFilter Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
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]>
Signed-off-by: zjgemi <[email protected]>
…_exploration_scheduler; add conf_filters to example Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: zjgemi <[email protected]>
| data = torch.load(input_file, map_location=torch.device("cpu")) | ||
| if not os.path.exists(output_dir): | ||
| os.makedirs(output_dir) | ||
| lengths = data["lengths"] | ||
| angles = data["angles"] | ||
| num_atoms = data["num_atoms"] | ||
| frac_coors = data["frac_coords"] | ||
| atom_types = data["atom_types"] | ||
|
|
||
| lengths_list = lengths.numpy() | ||
| angles_list = angles.numpy() | ||
| num_atoms_list = num_atoms | ||
| frac_coors_list = frac_coors.numpy() | ||
| atom_types_list = atom_types | ||
| if len(atom_types_list.shape) > 1: | ||
| atom_types_list = np.argmax(atom_types_list, axis=-1) + 1 | ||
|
|
||
| now_atom = 0 | ||
| for i in range(len(num_atoms_list)): | ||
| length = lengths_list[i] | ||
| angle = angles_list[i] | ||
| atom_num = num_atoms_list[i] | ||
|
|
||
| atom_type = atom_types_list[now_atom : now_atom + atom_num] | ||
| frac_coord = frac_coors_list[now_atom : now_atom + atom_num] | ||
| lattice = Lattice.from_parameters(*(length.tolist() + angle.tolist())) | ||
| structure = Structure( | ||
| lattice, atom_type, frac_coord, coords_are_cartesian=False | ||
| ) | ||
|
|
||
| filename = "%s.cif" % i | ||
| file_path = os.path.join(output_dir, filename) | ||
| structure.to(filename=file_path) | ||
| now_atom += atom_num |
There was a problem hiding this comment.
Add error handling for file operations.
Consider adding error handling for file operations to catch potential issues with file I/O.
You can wrap the file operations in a try-except block to handle exceptions like FileNotFoundError or IOError.
| except ValueError: | ||
| raise RuntimeError("Path of DiffCSP model not provided.") | ||
| model_path = args[i + 1] | ||
| subprocess.run(cmd, shell=True, check=True) |
There was a problem hiding this comment.
Improve subprocess command execution.
Use subprocess.run with additional parameters for better error handling and output management.
Apply this diff to improve the subprocess execution:
- subprocess.run(cmd, shell=True, check=True)
+ subprocess.run(cmd, shell=True, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)This change captures the command's output and errors, allowing for better debugging and logging.
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.
| subprocess.run(cmd, shell=True, check=True) | |
| subprocess.run(cmd, shell=True, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
| if not os.path.exists(output_dir): | ||
| os.makedirs(output_dir) |
There was a problem hiding this comment.
Use os.makedirs with exist_ok=True.
To simplify the code and avoid checking for the directory's existence, use os.makedirs with exist_ok=True.
Apply this diff to improve the code:
if not os.path.exists(output_dir):
- os.makedirs(output_dir)
+ os.makedirs(output_dir, exist_ok=True)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.
| if not os.path.exists(output_dir): | |
| os.makedirs(output_dir) | |
| os.makedirs(output_dir, exist_ok=True) |
Summary by CodeRabbit
New Features
variant_filterfor configuration filtering in exploration tasks.DiffCSPexploration style, enhancing exploration capabilities.DiffCSPtasks, includingDiffCSPTaskGroup.DiffCSPGenclass.DistanceConfFilterfor validating atomic configurations based on distance criteria.RunRelaxclass for structured execution of relaxation tasks.Bug Fixes
Tests
DistanceConfFilter,DiffCSPGen,RunRelax, andPrepRunDiffCSPto ensure functionality and integration.