-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get cell #186
Conversation
WalkthroughThe recent update enhances the codebase by introducing a versatile Changes
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 Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
structuretoolkit/common/__init__.py (1)
Line range hint
1-1
: Many imports in this file are unused. Consider removing them or adding them to the__all__
list if they are meant to be part of the public API.- from structuretoolkit.common.error import SymmetryError - from structuretoolkit.common.helper import ( - apply_strain, - center_coordinates_in_unit_cell, - get_extended_positions, - get_vertical_length, - get_wrapped_coordinates, - select_index, - ) - from structuretoolkit.common.pymatgen import ( - ase_to_pymatgen, - pymatgen_read_from_file, - pymatgen_to_ase, - ) - from structuretoolkit.common.pyscal import ase_to_pyscal - from structuretoolkit.common.phonopy import phonopy_to_atoms, atoms_to_phonopyAlso applies to: 3-3, 4-4, 5-5, 6-6, 7-7, 8-8, 12-12, 13-13, 14-14, 16-16, 17-17
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- structuretoolkit/init.py (1 hunks)
- structuretoolkit/common/init.py (1 hunks)
- structuretoolkit/common/helper.py (2 hunks)
- tests/test_helpers.py (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/test_helpers.py
Additional Context Used
Ruff (70)
structuretoolkit/__init__.py (55)
5-5:
structuretoolkit.analyse.find_mic
imported but unused; consider removing, adding to__all__
, or using a redundant alias
6-6:
structuretoolkit.analyse.find_solids
imported but unused; consider removing, adding to__all__
, or using a redundant alias
7-7:
structuretoolkit.analyse.get_adaptive_cna_descriptors
imported but unused; consider removing, adding to__all__
, or using a redundant alias
8-8:
structuretoolkit.analyse.get_average_of_unique_labels
imported but unused; consider removing, adding to__all__
, or using a redundant alias
9-9:
structuretoolkit.analyse.get_centro_symmetry_descriptors
imported but unused; consider removing, adding to__all__
, or using a redundant alias
10-10:
structuretoolkit.analyse.get_cluster_positions
imported but unused; consider removing, adding to__all__
, or using a redundant alias
11-11:
structuretoolkit.analyse.get_delaunay_neighbors
imported but unused; consider removing, adding to__all__
, or using a redundant alias
12-12:
structuretoolkit.analyse.get_diamond_structure_descriptors
imported but unused; consider removing, adding to__all__
, or using a redundant alias
13-13:
structuretoolkit.analyse.get_distances_array
imported but unused; consider removing, adding to__all__
, or using a redundant alias
14-14:
structuretoolkit.analyse.get_equivalent_atoms
imported but unused; consider removing, adding to__all__
, or using a redundant alias
15-15:
structuretoolkit.analyse.get_interstitials
imported but unused; consider removing, adding to__all__
, or using a redundant alias
16-16:
structuretoolkit.analyse.get_layers
imported but unused; consider removing, adding to__all__
, or using a redundant alias
17-17:
structuretoolkit.analyse.get_mean_positions
imported but unused; consider removing, adding to__all__
, or using a redundant alias
18-18:
structuretoolkit.analyse.get_neighborhood
imported but unused; consider removing, adding to__all__
, or using a redundant alias
19-19:
structuretoolkit.analyse.get_neighbors
imported but unused; consider removing, adding to__all__
, or using a redundant alias
20-20:
structuretoolkit.analyse.get_steinhardt_parameters
imported but unused; consider removing, adding to__all__
, or using a redundant alias
21-21:
structuretoolkit.analyse.get_strain
imported but unused; consider removing, adding to__all__
, or using a redundant alias
22-22:
structuretoolkit.analyse.get_symmetry
imported but unused; consider removing, adding to__all__
, or using a redundant alias
23-23:
structuretoolkit.analyse.get_voronoi_neighbors
imported but unused; consider removing, adding to__all__
, or using a redundant alias
24-24:
structuretoolkit.analyse.get_voronoi_vertices
imported but unused; consider removing, adding to__all__
, or using a redundant alias
25-25:
structuretoolkit.analyse.get_voronoi_volumes
imported but unused; consider removing, adding to__all__
, or using a redundant alias
30-30:
structuretoolkit.build.B2
imported but unused; consider removing, adding to__all__
, or using a redundant alias
31-31:
structuretoolkit.build.C14
imported but unused; consider removing, adding to__all__
, or using a redundant alias
32-32:
structuretoolkit.build.C15
imported but unused; consider removing, adding to__all__
, or using a redundant alias
33-33:
structuretoolkit.build.C36
imported but unused; consider removing, adding to__all__
, or using a redundant alias
34-34:
structuretoolkit.build.D03
imported but unused; consider removing, adding to__all__
, or using a redundant alias
35-35:
structuretoolkit.build.get_grainboundary_info
imported but unused; consider removing, adding to__all__
, or using a redundant alias
36-36:
structuretoolkit.build.get_high_index_surface_info
imported but unused; consider removing, adding to__all__
, or using a redundant alias
37-37:
structuretoolkit.build.grainboundary
imported but unused; consider removing, adding to__all__
, or using a redundant alias
38-38:
structuretoolkit.build.high_index_surface
imported but unused; consider removing, adding to__all__
, or using a redundant alias
39-39:
structuretoolkit.build.sqs_structures
imported but unused; consider removing, adding to__all__
, or using a redundant alias
44-44:
structuretoolkit.common.SymmetryError
imported but unused; consider removing, adding to__all__
, or using a redundant alias
45-45:
structuretoolkit.common.apply_strain
imported but unused; consider removing, adding to__all__
, or using a redundant alias
46-46:
structuretoolkit.common.ase_to_pymatgen
imported but unused; consider removing, adding to__all__
, or using a redundant alias
47-47:
structuretoolkit.common.ase_to_pyscal
imported but unused; consider removing, adding to__all__
, or using a redundant alias
48-48:
structuretoolkit.common.center_coordinates_in_unit_cell
imported but unused; consider removing, adding to__all__
, or using a redundant alias
49-49:
structuretoolkit.common.get_extended_positions
imported but unused; consider removing, adding to__all__
, or using a redundant alias
50-50:
structuretoolkit.common.get_vertical_length
imported but unused; consider removing, adding to__all__
, or using a redundant alias
51-51:
structuretoolkit.common.get_wrapped_coordinates
imported but unused; consider removing, adding to__all__
, or using a redundant alias
52-52:
structuretoolkit.common.pymatgen_to_ase
imported but unused; consider removing, adding to__all__
, or using a redundant alias
53-53:
structuretoolkit.common.select_index
imported but unused; consider removing, adding to__all__
, or using a redundant alias
54-54:
structuretoolkit.common.get_cell
imported but unused; consider removing, adding to__all__
, or using a redundant alias
58-58:
structuretoolkit.visualize.plot3d
imported but unused; consider removing, adding to__all__
, or using a redundant alias
62-62:
structuretoolkit.analyse.find_solids
imported but unused; consider removing, adding to__all__
, or using a redundant alias
63-63:
structuretoolkit.analyse.get_adaptive_cna_descriptors
imported but unused; consider removing, adding to__all__
, or using a redundant alias
64-64:
structuretoolkit.analyse.get_centro_symmetry_descriptors
imported but unused; consider removing, adding to__all__
, or using a redundant alias
65-65:
structuretoolkit.analyse.get_cluster_positions
imported but unused; consider removing, adding to__all__
, or using a redundant alias
66-66:
structuretoolkit.analyse.get_diamond_structure_descriptors
imported but unused; consider removing, adding to__all__
, or using a redundant alias
67-67:
structuretoolkit.analyse.get_equivalent_atoms
imported but unused; consider removing, adding to__all__
, or using a redundant alias
68-68:
structuretoolkit.analyse.get_steinhardt_parameters
imported but unused; consider removing, adding to__all__
, or using a redundant alias
69-69:
structuretoolkit.analyse.get_voronoi_volumes
imported but unused; consider removing, adding to__all__
, or using a redundant alias
74-74:
structuretoolkit.build.get_grainboundary_info
imported but unused; consider removing, adding to__all__
, or using a redundant alias
75-75:
structuretoolkit.build.get_high_index_surface_info
imported but unused; consider removing, adding to__all__
, or using a redundant alias
76-76:
structuretoolkit.build.grainboundary
imported but unused; consider removing, adding to__all__
, or using a redundant alias
77-77:
structuretoolkit.build.sqs_structures
imported but unused; consider removing, adding to__all__
, or using a redundant aliasstructuretoolkit/common/__init__.py (14)
1-1:
structuretoolkit.common.error.SymmetryError
imported but unused; consider removing, adding to__all__
, or using a redundant alias
3-3:
structuretoolkit.common.helper.apply_strain
imported but unused; consider removing, adding to__all__
, or using a redundant alias
4-4:
structuretoolkit.common.helper.center_coordinates_in_unit_cell
imported but unused; consider removing, adding to__all__
, or using a redundant alias
5-5:
structuretoolkit.common.helper.get_extended_positions
imported but unused; consider removing, adding to__all__
, or using a redundant alias
6-6:
structuretoolkit.common.helper.get_vertical_length
imported but unused; consider removing, adding to__all__
, or using a redundant alias
7-7:
structuretoolkit.common.helper.get_wrapped_coordinates
imported but unused; consider removing, adding to__all__
, or using a redundant alias
8-8:
structuretoolkit.common.helper.select_index
imported but unused; consider removing, adding to__all__
, or using a redundant alias
9-9:
structuretoolkit.common.helper.get_cell
imported but unused; consider removing, adding to__all__
, or using a redundant alias
12-12:
structuretoolkit.common.pymatgen.ase_to_pymatgen
imported but unused; consider removing, adding to__all__
, or using a redundant alias
13-13:
structuretoolkit.common.pymatgen.pymatgen_read_from_file
imported but unused; consider removing, adding to__all__
, or using a redundant alias
14-14:
structuretoolkit.common.pymatgen.pymatgen_to_ase
imported but unused; consider removing, adding to__all__
, or using a redundant alias
16-16:
structuretoolkit.common.pyscal.ase_to_pyscal
imported but unused; consider removing, adding to__all__
, or using a redundant alias
17-17:
structuretoolkit.common.phonopy.phonopy_to_atoms
imported but unused; consider removing, adding to__all__
, or using a redundant alias
17-17:
structuretoolkit.common.phonopy.atoms_to_phonopy
imported but unused; consider removing, adding to__all__
, or using a redundant aliasstructuretoolkit/common/helper.py (1)
3-3:
ase.data.atomic_numbers
imported but unused
Additional comments not posted (3)
structuretoolkit/common/__init__.py (1)
9-9
: The addition ofget_cell
to the__all__
list correctly exposes it as a public API.structuretoolkit/__init__.py (1)
54-54
: The addition ofget_cell
to the__all__
list correctly exposes it as a public API.structuretoolkit/common/helper.py (1)
231-250
: The implementation ofget_cell
function is correct and handles different input types effectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, I didn't mean to imply this existed already -- afaik it does not. This is exactly what I had in mind!
I think you still need a safety valve to fail cleanly in the event the array has a shape other than (3,) or (3,3).
Also a tuple should be ok too.
Probably it will be cleanest to isinstance on tuple and list and convert them to an array if so, then isinstance on array and do the shape stuff you already have + clean railing on unexpected shapes.
In the previous version it was anyway failing with anything other than float, (3,) or (3,3), but now I also added a clear error message. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- structuretoolkit/common/helper.py (2 hunks)
- tests/test_helpers.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/test_helpers.py
Additional Context Used
Ruff (1)
structuretoolkit/common/helper.py (1)
3-3:
ase.data.atomic_numbers
imported but unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- structuretoolkit/common/helper.py (2 hunks)
- tests/test_helpers.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/test_helpers.py
Additional Context Used
Ruff (1)
structuretoolkit/common/helper.py (1)
3-3:
ase.data.atomic_numbers
imported but unused
structuretoolkit/common/helper.py
Outdated
def get_cell(cell: Union[Atoms, list, np.ndarray, float]): | ||
""" | ||
Get cell of an ase structure, or convert a float or a (3,)-array into a | ||
orthogonal cell. | ||
|
||
Args: | ||
cell (Atoms|ndarray|list|float|tuple): Cell | ||
|
||
Returns: | ||
(3, 3)-array: Cell | ||
""" | ||
if isinstance(cell, Atoms): | ||
return cell.cell | ||
# Convert float into (3,)-array. No effect if it is (3,3)-array or | ||
# (3,)-array. Raises error if the shape is not correct | ||
try: | ||
cell = cell * np.ones(3) | ||
except ValueError: | ||
raise ValueError( | ||
"cell must be a float, (3,)-ndarray/list/tuple or" | ||
" (3,3)-ndarray/list/tuple" | ||
) | ||
|
||
if np.shape(cell) == (3, 3): | ||
return cell | ||
# Convert (3,)-array into (3,3)-array. Raises error if the shape is wrong | ||
try: | ||
return cell * np.eye(3) | ||
except ValueError: | ||
raise ValueError( | ||
"cell must be a float, (3,)-ndarray/list/tuple or" | ||
" (3,3)-ndarray/list/tuple" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor get_cell
for enhanced clarity and error handling.
231 def get_cell(cell: Union[Atoms, list, np.ndarray, float]):
232 """
233 Get cell of an ase structure, or convert a float or a (3,)-array into an
234 orthogonal cell.
235
236 Args:
237 cell (Atoms|ndarray|list|float|tuple): Cell
238
239 Returns:
240 (3, 3)-array: Cell
241 """
242 if isinstance(cell, Atoms):
243 return cell.cell
244 # Convert float into (3,)-array. No effect if it is (3,3)-array or
245 # (3,)-array. Raises error if the shape is not correct
246 try:
247 cell = cell * np.ones(3)
248 except ValueError:
249 raise ValueError(
250 "cell must be a float, (3,)-ndarray/list/tuple or"
251 " (3,3)-ndarray/list/tuple"
252 )
253
254 if np.shape(cell) == (3, 3):
255 return cell
256 # Convert (3,)-array into (3,3)-array. Raises error if the shape is wrong
257 try:
258 return cell * np.eye(3)
259 except ValueError:
260 raise ValueError(
261 "cell must be a float, (3,)-ndarray/list/tuple or"
262 " (3,3)-ndarray/list/tuple"
263 )
Consider enhancing the error messages to include the actual received input type or shape for better debugging.
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.
def get_cell(cell: Union[Atoms, list, np.ndarray, float]): | |
""" | |
Get cell of an ase structure, or convert a float or a (3,)-array into a | |
orthogonal cell. | |
Args: | |
cell (Atoms|ndarray|list|float|tuple): Cell | |
Returns: | |
(3, 3)-array: Cell | |
""" | |
if isinstance(cell, Atoms): | |
return cell.cell | |
# Convert float into (3,)-array. No effect if it is (3,3)-array or | |
# (3,)-array. Raises error if the shape is not correct | |
try: | |
cell = cell * np.ones(3) | |
except ValueError: | |
raise ValueError( | |
"cell must be a float, (3,)-ndarray/list/tuple or" | |
" (3,3)-ndarray/list/tuple" | |
) | |
if np.shape(cell) == (3, 3): | |
return cell | |
# Convert (3,)-array into (3,3)-array. Raises error if the shape is wrong | |
try: | |
return cell * np.eye(3) | |
except ValueError: | |
raise ValueError( | |
"cell must be a float, (3,)-ndarray/list/tuple or" | |
" (3,3)-ndarray/list/tuple" | |
) | |
def get_cell(cell: Union[Atoms, list, np.ndarray, float]): | |
""" | |
Get cell of an ase structure, or convert a float or a (3,)-array into an | |
orthogonal cell. | |
Args: | |
cell (Atoms|ndarray|list|float|tuple): Cell | |
Returns: | |
(3, 3)-array: Cell | |
""" | |
if isinstance(cell, Atoms): | |
return cell.cell | |
# Convert float into (3,)-array. No effect if it is (3,3)-array or | |
# (3,)-array. Raises error if the shape is not correct | |
try: | |
cell = cell * np.ones(3) | |
except ValueError: | |
raise ValueError( | |
f"cell must be a float, (3,)-ndarray/list/tuple or" | |
f" (3,3)-ndarray/list/tuple, but got {type(cell)} with shape {np.shape(cell)}" | |
) | |
if np.shape(cell) == (3, 3): | |
return cell | |
# Convert (3,)-array into (3,3)-array. Raises error if the shape is wrong | |
try: | |
return cell * np.eye(3) | |
except ValueError: | |
raise ValueError( | |
f"cell must be a float, (3,)-ndarray/list/tuple or" | |
f" (3,3)-ndarray/list/tuple, but got {type(cell)} with shape {np.shape(cell)}" | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- structuretoolkit/visualize.py (2 hunks)
Additional comments not posted (1)
structuretoolkit/visualize.py (1)
171-171
: Integration ofget_cell
function in_draw_box_plotly
looks good.Verification successful
The
get_cell
function is properly integrated and used across the system. The search results show its usage in various files, including tests and other modules, confirming its integration.
tests/test_helpers.py
: Multiple test cases forget_cell
.structuretoolkit/visualize.py
: Usage in the_draw_box_plotly
function.structuretoolkit/common/helper.py
: Definition ofget_cell
.structuretoolkit/common/phonopy.py
andstructuretoolkit/analyse/symmetry.py
: Other usages ofget_cell
.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `get_cell` function is properly integrated and used across the system. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python $'get_cell'Length of output: 58
Script:
#!/bin/bash # Description: Verify that `get_cell` function is properly integrated and used across the system. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py 'get_cell'Length of output: 1077
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type hint update is mandatory, the other comment is merely a suggestion. No need for re-review though and both these are anyhow minor things so this gets my approval.
structuretoolkit/common/helper.py
Outdated
raise ValueError( | ||
"cell must be a float, (3,)-ndarray/list/tuple or" | ||
" (3,3)-ndarray/list/tuple" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guy gets both duplicated and doesn't have a friendly custom class to test against; you might be able to kill both birds with one stone following a pattern like:
class CustomError(ValueError):
def __init__(self, /, msg="My Custom message"):
super().__init__(msg)
And then raising that in both places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now gave it a short try, but after I implemented what Code Rabbit suggested, it looked complicated to pass arguments there, and since in terms of the number of lines it doesn't change much, I kept Code Rabbit's suggestion
Co-authored-by: Liam Huber <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- structuretoolkit/common/helper.py (2 hunks)
Additional Context Used
Ruff (1)
structuretoolkit/common/helper.py (1)
3-3:
ase.data.atomic_numbers
imported but unused
structuretoolkit/common/helper.py
Outdated
def get_cell(cell: Union[Atoms, list, tuple, np.ndarray, float]): | ||
""" | ||
Get cell of an ase structure, or convert a float or a (3,)-array into a | ||
orthogonal cell. | ||
|
||
Args: | ||
cell (Atoms|ndarray|list|float|tuple): Cell | ||
|
||
Returns: | ||
(3, 3)-array: Cell | ||
""" | ||
if isinstance(cell, Atoms): | ||
return cell.cell | ||
# Convert float into (3,)-array. No effect if it is (3,3)-array or | ||
# (3,)-array. Raises error if the shape is not correct | ||
try: | ||
cell = cell * np.ones(3) | ||
except ValueError: | ||
raise ValueError( | ||
"cell must be a float, (3,)-ndarray/list/tuple or" | ||
" (3,3)-ndarray/list/tuple" | ||
) | ||
|
||
if np.shape(cell) == (3, 3): | ||
return cell | ||
# Convert (3,)-array into (3,3)-array. Raises error if the shape is wrong | ||
try: | ||
return cell * np.eye(3) | ||
except ValueError: | ||
raise ValueError( | ||
"cell must be a float, (3,)-ndarray/list/tuple or" | ||
" (3,3)-ndarray/list/tuple" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor get_cell
for enhanced clarity and error handling.
231 def get_cell(cell: Union[Atoms, list, tuple, np.ndarray, float]):
232 """
233 Get cell of an ase structure, or convert a float or a (3,)-array into an
234 orthogonal cell.
235
236 Args:
237 cell (Atoms|ndarray|list|float|tuple): Cell
238
239 Returns:
240 (3, 3)-array: Cell
241 """
242 if isinstance(cell, Atoms):
243 return cell.cell
244 # Convert float into (3,)-array. No effect if it is (3,3)-array or
245 # (3,)-array. Raises error if the shape is not correct
246 try:
247- cell = cell * np.ones(3)
+ cell = np.full(3, cell)
248 except ValueError:
249 raise ValueError(
250- "cell must be a float, (3,)-ndarray/list/tuple or"
251- " (3,3)-ndarray/list/tuple"
+ f"Invalid cell type or shape: {type(cell).__name__}, {np.shape(cell)}"
252 )
253
254 if np.shape(cell) == (3, 3):
255 return cell
256 # Convert (3,)-array into (3,3)-array. Raises error if the shape is wrong
257 try:
258 return cell * np.eye(3)
259 except ValueError:
260 raise ValueError(
261- "cell must be a float, (3,)-ndarray/list/tuple or"
262- " (3,3)-ndarray/list/tuple"
+ f"Invalid cell type or shape: {type(cell).__name__}, {np.shape(cell)}"
263 )
Consider enhancing the error messages to include the actual received input type or shape for better debugging.
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.
def get_cell(cell: Union[Atoms, list, tuple, np.ndarray, float]): | |
""" | |
Get cell of an ase structure, or convert a float or a (3,)-array into a | |
orthogonal cell. | |
Args: | |
cell (Atoms|ndarray|list|float|tuple): Cell | |
Returns: | |
(3, 3)-array: Cell | |
""" | |
if isinstance(cell, Atoms): | |
return cell.cell | |
# Convert float into (3,)-array. No effect if it is (3,3)-array or | |
# (3,)-array. Raises error if the shape is not correct | |
try: | |
cell = cell * np.ones(3) | |
except ValueError: | |
raise ValueError( | |
"cell must be a float, (3,)-ndarray/list/tuple or" | |
" (3,3)-ndarray/list/tuple" | |
) | |
if np.shape(cell) == (3, 3): | |
return cell | |
# Convert (3,)-array into (3,3)-array. Raises error if the shape is wrong | |
try: | |
return cell * np.eye(3) | |
except ValueError: | |
raise ValueError( | |
"cell must be a float, (3,)-ndarray/list/tuple or" | |
" (3,3)-ndarray/list/tuple" | |
) | |
def get_cell(cell: Union[Atoms, list, tuple, np.ndarray, float]): | |
""" | |
Get cell of an ase structure, or convert a float or a (3,)-array into a | |
orthogonal cell. | |
Args: | |
cell (Atoms|ndarray|list|float|tuple): Cell | |
Returns: | |
(3, 3)-array: Cell | |
""" | |
if isinstance(cell, Atoms): | |
return cell.cell | |
# Convert float into (3,)-array. No effect if it is (3,3)-array or | |
# (3,)-array. Raises error if the shape is not correct | |
try: | |
cell = np.full(3, cell) | |
except ValueError: | |
raise ValueError( | |
f"Invalid cell type or shape: {type(cell).__name__}, {np.shape(cell)}" | |
) | |
if np.shape(cell) == (3, 3): | |
return cell | |
# Convert (3,)-array into (3,3)-array. Raises error if the shape is wrong | |
try: | |
return cell * np.eye(3) | |
except ValueError: | |
raise ValueError( | |
f"Invalid cell type or shape: {type(cell).__name__}, {np.shape(cell)}" | |
) |
…t_cell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- structuretoolkit/common/helper.py (2 hunks)
Additional Context Used
Ruff (1)
structuretoolkit/common/helper.py (1)
3-3:
ase.data.atomic_numbers
imported but unused
Additional comments not posted (1)
structuretoolkit/common/helper.py (1)
249-261
: Enhanced error messages inget_cell
function.The inclusion of the actual input type and shape in the error messages is a good practice for debugging. This change improves the clarity and usefulness of the error messages.
@@ -2,7 +2,7 @@ | |||
from ase.atoms import Atoms | |||
from ase.data import atomic_numbers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused import to clean up the code.
- from ase.data import atomic_numbers
This import is unused and should be removed to clean up the codebase.
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.
from ase.data import atomic_numbers |
def get_cell(cell: Union[Atoms, list, tuple, np.ndarray, float]): | ||
""" | ||
Get cell of an ase structure, or convert a float or a (3,)-array into a | ||
orthogonal cell. | ||
|
||
Args: | ||
cell (Atoms|ndarray|list|float|tuple): Cell | ||
|
||
Returns: | ||
(3, 3)-array: Cell | ||
""" | ||
if isinstance(cell, Atoms): | ||
return cell.cell | ||
# Convert float into (3,)-array. No effect if it is (3,3)-array or | ||
# (3,)-array. Raises error if the shape is not correct | ||
try: | ||
cell = cell * np.ones(3) | ||
except ValueError: | ||
raise ValueError( | ||
f"Invalid cell type or shape: {type(cell).__name__}, {np.shape(cell)}" | ||
) | ||
|
||
if np.shape(cell) == (3, 3): | ||
return cell | ||
# Convert (3,)-array into (3,3)-array. Raises error if the shape is wrong | ||
try: | ||
return cell * np.eye(3) | ||
except ValueError: | ||
raise ValueError( | ||
f"Invalid cell type or shape: {type(cell).__name__}, {np.shape(cell)}" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor get_cell
for clarity and efficiency.
247- cell = cell * np.ones(3)
+ cell = np.full(3, cell)
Consider using np.full
instead of multiplying by np.ones(3)
for clarity and efficiency. This change was suggested in a previous review and is still applicable.
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.
def get_cell(cell: Union[Atoms, list, tuple, np.ndarray, float]): | |
""" | |
Get cell of an ase structure, or convert a float or a (3,)-array into a | |
orthogonal cell. | |
Args: | |
cell (Atoms|ndarray|list|float|tuple): Cell | |
Returns: | |
(3, 3)-array: Cell | |
""" | |
if isinstance(cell, Atoms): | |
return cell.cell | |
# Convert float into (3,)-array. No effect if it is (3,3)-array or | |
# (3,)-array. Raises error if the shape is not correct | |
try: | |
cell = cell * np.ones(3) | |
except ValueError: | |
raise ValueError( | |
f"Invalid cell type or shape: {type(cell).__name__}, {np.shape(cell)}" | |
) | |
if np.shape(cell) == (3, 3): | |
return cell | |
# Convert (3,)-array into (3,3)-array. Raises error if the shape is wrong | |
try: | |
return cell * np.eye(3) | |
except ValueError: | |
raise ValueError( | |
f"Invalid cell type or shape: {type(cell).__name__}, {np.shape(cell)}" | |
) | |
try: | |
cell = np.full(3, cell) |
Following @liamhuber's suggestion, I created a converter. He says the function is already there, although I couldn't find it, but it doesn't mean that it's definitely no there, so I might end up closing this PR.
Summary by CodeRabbit
New Features
Refactor
_draw_box_plotly
function to use the new cell retrieval method.Tests