add distance conf filter #250
Conversation
Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
WalkthroughWalkthroughThe recent changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Args
participant Submit
participant Scheduler
participant Renderer
participant DistanceFilter
User->>Args: Call lmp_args(filters)
Args->>Submit: Pass filters to get_conf_filters()
Submit->>Scheduler: Integrate conf_filters
Scheduler->>Renderer: Invoke get_confs(conf_filters)
Renderer->>DistanceFilter: Validate configurations
DistanceFilter-->>Renderer: Return validation result
Renderer-->>Scheduler: Return filtered subsystems
Scheduler-->>User: Provide exploration results
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional context usedRuff
Additional comments not posted (5)
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 (
|
![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/exploration/selector/conf_filter.py (1)
70-70: Compatibility Issue:ff.checkdoes not support atom namesThe
ff.checkfunction indpgen2/exploration/selector/conf_filter.pycurrently acceptsatom_typesas a parameter. To ensure compatibility with atom names, the function's implementation needs to be reviewed and potentially updated to handleatom_names.
- Location:
dpgen2/exploration/selector/conf_filter.py, functioncheckPlease verify the function's logic and update it as necessary to accommodate atom names.
Analysis chain
Verify compatibility of atom names with
ff.check.The change from atom types to atom names enhances clarity but ensure that
ff.checkis compatible with receiving atom names instead of types.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ff.check` can handle atom names instead of atom types. # Test: Search for the `ff.check` function definition to verify its parameter handling. rg --type python -A 5 'def check' | rg 'atom_names'Length of output: 82
Script:
#!/bin/bash # Description: Verify that `ff.check` can handle atom names instead of atom types. # Test: Search for the `ff.check` function definition to verify its parameter handling. rg --type py -A 5 'def check' | rg 'atom_names'Length of output: 137
Script:
#!/bin/bash # Description: Locate the `ff.check` function definition to verify its parameter handling. # Test: Search for the `ff.check` function definition to understand its parameters. rg --type py -A 10 'def check'Length of output: 10684
dpgen2/exploration/selector/distance_conf_filter.py (1)
128-193: Consider removing debug print statements.The
checkmethod contains debug print statements that may not be suitable for production code.- print("Inclined box") - print("One side is 5 larger than another") - print( - f"Dangerous close for {type_i} - {type_j}, {dist:.5f} less than {dr:.5f}" - ) - print("Valid structure")Consider using logging instead of print statements for better control over output.
dpgen2/entrypoint/submit.py (1)
280-288: Ensureconf_filter_stylescovers all expected types.Currently,
conf_filter_stylesonly contains the key"distance". Consider adding error handling in theget_conf_filtersfunction for unexpected types and ensure that all expected types are included inconf_filter_styles.
- Location to update:
dpgen2/exploration/selector/__init__.pywhereconf_filter_stylesis defined.- Function to enhance:
get_conf_filtersindpgen2/entrypoint/submit.py.Analysis chain
Verify
conf_filter_stylescomprehensiveness.Ensure that the
conf_filter_stylesdictionary includes all expected types to prevent runtime errors.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all expected types are covered by `conf_filter_styles`. # Test: Search for all keys used with `conf_filter_styles`. Expect: All expected types are covered. rg --type python -A 1 'conf_filter_styles\[[^\]]+\]'Length of output: 85
Script:
#!/bin/bash # Description: Verify that all expected types are covered by `conf_filter_styles` using ast-grep. # Test: Search for all keys used with `conf_filter_styles`. Expect: All expected types are covered. ast-grep --lang python --pattern 'conf_filter_styles[$_]($$_)'Length of output: 161
Script:
#!/bin/bash # Description: Find the definition of `conf_filter_styles` and list its keys. # Search for the definition of `conf_filter_styles` and print its contents. rg --type python 'conf_filter_styles\s*=' -A 5Length of output: 79
Script:
#!/bin/bash # Description: Find the definition of `conf_filter_styles` and list its keys using ast-grep. # Search for the definition of `conf_filter_styles`. ast-grep --lang python --pattern 'conf_filter_styles = $_'Length of output: 250
| conf_filter = conf_filter_styles[c.pop("type")](**c) | ||
| conf_filters.add(conf_filter) |
There was a problem hiding this comment.
Add error handling for unexpected types.
Consider adding error handling to manage unexpected types that may not be present in conf_filter_styles.
try:
conf_filter = conf_filter_styles[c.pop("type")](**c)
except KeyError as e:
raise ValueError(f"Unexpected filter type: {e}")| ) | ||
|
|
||
| from .context import ( | ||
| dpgen2, |
There was a problem hiding this comment.
Remove unused import.
The import statement for .context.dpgen2 is not used in the file.
- from .context import (
- dpgen2,
- )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.
| dpgen2, | |
| from .context import ( | |
| ) |
Tools
Ruff
12-12:
.context.dpgen2imported but unusedRemove unused import:
.context.dpgen2(F401)
| import os | ||
| import shutil | ||
|
|
||
| import dpdata |
There was a problem hiding this comment.
Remove unused imports.
The imports os, shutil, and dpdata are not used in the file.
- import os
- import shutil
- 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 os | |
| import shutil | |
| import dpdata |
Tools
Ruff
1-1:
osimported but unusedRemove unused import:
os(F401)
2-2:
shutilimported but unusedRemove unused import:
shutil(F401)
4-4:
dpdataimported but unusedRemove unused import:
dpdata(F401)
| if i != j: | ||
| if values[i] > multiple * values[j]: |
There was a problem hiding this comment.
Combine nested if statements.
The nested if statements can be combined for better readability.
- 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.
| if i != j: | |
| if values[i] > multiple * values[j]: | |
| if i != j and values[i] > multiple * values[j]: |
Tools
Ruff
119-120: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #250 +/- ##
==========================================
+ Coverage 83.75% 83.87% +0.12%
==========================================
Files 97 98 +1
Lines 5318 5433 +115
==========================================
+ Hits 4454 4557 +103
- Misses 864 876 +12 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
| 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 contains hardcoded values. Consider externalizing this data to a configuration file or database to enhance flexibility and maintainability.
| class DistanceConfFilter(ConfFilter): | ||
| def __init__( | ||
| self, custom_safe_dist=None, safe_dist_ratio=1.0, theta=60.0, length_ratio=5.0 | ||
| ): | ||
| self.custom_safe_dist = custom_safe_dist if custom_safe_dist is not None else {} | ||
| self.safe_dist_ratio = safe_dist_ratio | ||
| self.theta = theta | ||
| self.length_ratio = length_ratio | ||
|
|
||
| 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.
Replace print statements with logging.
Consider using a logging framework instead of print statements to provide better control over the output and integrate with existing logging systems.
- print("Inclined box")
+ logger.warning("Inclined box")- print("One side is %s larger than another" % self.length_ratio)
+ logger.warning("One side is %s larger than another", self.length_ratio)- print(f"Dangerous close for {type_i} - {type_j}, {dist:.5f} less than {dr:.5f}")
+ logger.warning("Dangerous close for %s - %s, %.5f less than %.5f", type_i, type_j, dist, dr)- print("Valid structure")
+ logger.info("Valid structure")import logging
logger = logging.getLogger(__name__)| Argument( | ||
| "filters", | ||
| list, | ||
| [], | ||
| [variant_filter()], | ||
| optional=True, | ||
| default=[], | ||
| doc=doc_filters, |
There was a problem hiding this comment.
Avoid mutable default arguments.
Using a mutable default argument like a list can lead to unexpected behavior. Consider using None and initializing inside the function.
- def lmp_args(filters: List[dict] = [], optional=True, doc=doc_filters):
+ def lmp_args(filters: List[dict] = None, optional=True, doc=doc_filters):
+ if filters is None:
+ filters = []
- def caly_args(filters: List[dict] = [], optional=True, doc=doc_filters):
+ def caly_args(filters: List[dict] = None, optional=True, doc=doc_filters):
+ if filters is None:
+ filters = []Also applies to: 346-354
| 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.
Optimize dictionary key iteration.
Use for kk in conf_filter_styles: instead of for kk in conf_filter_styles.keys(): for better 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)
… BoxSkewnessConfFilter Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
| class DistanceConfFilter(ConfFilter): | ||
| def __init__(self, custom_safe_dist=None, safe_dist_ratio=1.0): | ||
| self.custom_safe_dist = custom_safe_dist if custom_safe_dist is not None else {} | ||
| self.safe_dist_ratio = safe_dist_ratio | ||
|
|
||
| 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), | ||
| ) | ||
|
|
||
| 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 | ||
|
|
||
| return True | ||
|
|
There was a problem hiding this comment.
Enhance DistanceConfFilter with logging and configuration.
The DistanceConfFilter class uses print statements for output. Replace these with logging for better control and integration with logging systems. Additionally, consider externalizing configuration values like safe_dist_dict.
import logging
logger = logging.getLogger(__name__)- print(
- f"Dangerous close for {type_i} - {type_j}, {dist:.5f} less than {dr:.5f}"
- )
+ logger.warning(
+ "Dangerous close for %s - %s, %.5f less than %.5f", type_i, type_j, dist, dr
+ )| class BoxLengthFilter(ConfFilter): | ||
| def __init__(self, length_ratio=5.0): | ||
| self.length_ratio = length_ratio | ||
|
|
||
| def check( | ||
| self, | ||
| coords: np.ndarray, | ||
| cell: np.ndarray, | ||
| atom_types: np.ndarray, | ||
| nopbc: bool, | ||
| ): | ||
| from ase import ( | ||
| Atoms, | ||
| ) | ||
|
|
||
| atom_names = list(safe_dist_dict) | ||
| 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 | ||
|
|
||
| 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 |
There was a problem hiding this comment.
Replace print statements with logging in BoxLengthFilter.
Replace the print statement with logging to provide better control over the output.
- print("One side is %s larger than another" % self.length_ratio)
+ logger.warning("One side is %s larger than another", self.length_ratio)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.
| class BoxLengthFilter(ConfFilter): | |
| def __init__(self, length_ratio=5.0): | |
| self.length_ratio = length_ratio | |
| def check( | |
| self, | |
| coords: np.ndarray, | |
| cell: np.ndarray, | |
| atom_types: np.ndarray, | |
| nopbc: bool, | |
| ): | |
| from ase import ( | |
| Atoms, | |
| ) | |
| atom_names = list(safe_dist_dict) | |
| 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 | |
| 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 | |
| class BoxLengthFilter(ConfFilter): | |
| def __init__(self, length_ratio=5.0): | |
| self.length_ratio = length_ratio | |
| def check( | |
| self, | |
| coords: np.ndarray, | |
| cell: np.ndarray, | |
| atom_types: np.ndarray, | |
| nopbc: bool, | |
| ): | |
| from ase import ( | |
| Atoms, | |
| ) | |
| atom_names = list(safe_dist_dict) | |
| 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 | |
| a = cell[0][0] | |
| b = cell[1][1] | |
| c = cell[2][2] | |
| if check_multiples(a, b, c, self.length_ratio): | |
| logger.warning("One side is %s larger than another", self.length_ratio) | |
| return False |
| class BoxSkewnessConfFilter(ConfFilter): | ||
| def __init__(self, theta=60.0): | ||
| self.theta = theta | ||
|
|
||
| def check( | ||
| self, | ||
| coords: np.ndarray, | ||
| cell: np.ndarray, | ||
| atom_types: np.ndarray, | ||
| nopbc: bool, | ||
| ): | ||
| from ase import ( | ||
| Atoms, | ||
| ) | ||
|
|
||
| atom_names = list(safe_dist_dict) | ||
| 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 | ||
| return True |
There was a problem hiding this comment.
Replace print statements with logging in BoxSkewnessConfFilter.
Replace the print statement with logging to provide better control over the output.
- print("Inclined box")
+ logger.warning("Inclined box")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.
| class BoxSkewnessConfFilter(ConfFilter): | |
| def __init__(self, theta=60.0): | |
| self.theta = theta | |
| def check( | |
| self, | |
| coords: np.ndarray, | |
| cell: np.ndarray, | |
| atom_types: np.ndarray, | |
| nopbc: bool, | |
| ): | |
| from ase import ( | |
| Atoms, | |
| ) | |
| atom_names = list(safe_dist_dict) | |
| 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 | |
| return True | |
| class BoxSkewnessConfFilter(ConfFilter): | |
| def __init__(self, theta=60.0): | |
| self.theta = theta | |
| def check( | |
| self, | |
| coords: np.ndarray, | |
| cell: np.ndarray, | |
| atom_types: np.ndarray, | |
| nopbc: bool, | |
| ): | |
| from ase import ( | |
| Atoms, | |
| ) | |
| atom_names = list(safe_dist_dict) | |
| 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] | |
| ): | |
| logger.warning("Inclined box") | |
| return False | |
| return True |
Signed-off-by: zjgemi <[email protected]>
for more information, see https://pre-commit.ci
| import unittest | ||
|
|
||
| import dpdata | ||
| import numpy as np |
There was a problem hiding this comment.
Remove unused imports.
The imports for numpy and .context.dpgen2 are not used in the file.
- import numpy as np- from .context import (
- dpgen2,
- )Also applies to: 14-14
Tools
Ruff
5-5:
numpyimported but unusedRemove unused import:
numpy(F401)
Signed-off-by: zjgemi <[email protected]>
Signed-off-by: zjgemi <[email protected]>
Signed-off-by: zjgemi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
dpgen2/exploration/selector/distance_conf_filter.py (1)
1-14: Remove unused imports.The following imports appear to be unused in the file and should be removed to clean up the code:
loggingcopytypingAnalysis chain
Remove unused imports.
The imports
dargs,dpdata, anddeepcopyare used, but ensure thatnumpyand specific imports fromaseare necessary. Remove any unused imports to clean up the code.- import numpy as npRun the following script to verify the usage of imports:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of imports in the file. # Test: Check for unused imports. Expect: No output if all imports are used. rg --type python 'import' dpgen2/exploration/selector/distance_conf_filter.py | grep -v -E 'dargs|dpdata|deepcopy|ase'Length of output: 149
Script:
#!/bin/bash # Description: Verify the usage of imports in the file. # Corrected Test: Check for unused imports by searching for import statements and verifying their usage. rg 'import ' dpgen2/exploration/selector/distance_conf_filter.py | grep -v -E 'dargs|dpdata|deepcopy|ase|numpy'Length of output: 184
Summary by CodeRabbit
New Features
filtersargument for configuration customization in existing functions.Tests