fix "ValueError: could not convert string to float: 'ATOMIC_POSITIONS'" #721
Conversation
Signed-off-by: Hao-Jen You <[email protected]>
WalkthroughWalkthroughThe pull request introduces significant modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant get_block
participant get_cell
User->>get_block: Call with lines and start_marker
get_block->>get_block: Extract lines until blank or "&"
get_block-->>User: Return extracted block
User->>get_cell: Call with lines
get_cell->>get_cell: Check ibrav value
alt ibrav found
get_cell->>get_cell: Process CELL_PARAMETERS
else ibrav not found
get_cell-->>User: Raise RuntimeError
end
Tip OpenAI O1 model for chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
for more information, see https://pre-commit.ci
CodSpeed Performance ReportMerging #721 will not alter performanceComparing Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- dpdata/qe/scf.py (1 hunks)
Additional context used
Ruff
dpdata/qe/scf.py
32-32: Redefinition of unused
get_blockfrom line 15(F811)
50-50: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
Additional comments not posted (2)
dpdata/qe/scf.py (2)
15-29: Refactoring looks good!The refactoring of
get_blockfunction improves the clarity and efficiency by:
- Consolidating two versions into one.
- Accepting a
start_markerparameter.- Handling the extraction of lines until a blank line or a line starting with "&" is encountered.
Great job!
Line range hint
50-81: Changes improve robustness and maintainability.The changes to the
get_cellfunction improve the robustness and maintainability of the code by:
- Streamlining the logic for handling the
ibravparameter.- Raising a
RuntimeErrorif theibravvalue is not found, ensuring that the function fails fast when encountering unexpected input.- Updating the handling of the
CELL_PARAMETERSblock to ensure that it checks for the correct units (Angstrom) more robustly.- Making the extraction of numerical values from the lines clearer with the use of list comprehensions.
These changes clarify the control flow and error handling, enhancing the overall quality of the code.
Tools
Ruff
32-32: Redefinition of unused
get_blockfrom line 15(F811)
50-50: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
72-72: Yoda condition detected
Rewrite as
line[0] == "a"(SIM300)
74-74: Yoda condition detected
Rewrite as
line[0] == "celldm(1)"(SIM300)
| ret = [] | ||
| for idx, ii in enumerate(lines): | ||
| if "ibrav" in ii: | ||
| for idx, line in enumerate(lines): |
There was a problem hiding this comment.
Rename the unused loop control variable idx to _idx.
The loop control variable idx is not used within the loop body.
Rename idx to _idx to indicate that it is intentionally unused:
- for idx, line in enumerate(lines):
+ for _idx, line in enumerate(lines):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 idx, line in enumerate(lines): | |
| for _idx, line in enumerate(lines): |
Tools
Ruff
50-50: Loop control variable
idxnot used within loop bodyRename unused
idxto_idx(B007)
| return block | ||
|
|
||
|
|
||
| def get_block(lines, start_marker, skip=0): |
There was a problem hiding this comment.
Remove the unused get_block function.
There is a redefinition of the get_block function at line 32, which is unused in the code.
Remove the unused function definition:
-def get_block(lines, start_marker, skip=0):
- start_idx = None
- for idx, line in enumerate(lines):
- if start_marker in line:
- start_idx = idx + 1 + skip
- break
- if start_idx is None:
- raise RuntimeError(f"{start_marker} not found in the input lines.")
-
- block = []
- for line in lines[start_idx:]:
- if line.strip() == "" or line.strip().startswith("&"):
- break
- block.append(line.strip())
- return blockCommittable suggestion was skipped due to low confidence.
Tools
Ruff
32-32: Redefinition of unused
get_blockfrom line 15(F811)
| return block | ||
|
|
||
|
|
||
| def get_block(lines, start_marker, skip=0): |
There was a problem hiding this comment.
plz avoid redefining functions.
| for line in lines: | ||
| line = line.replace("=", " ").replace(",", "").split() |
There was a problem hiding this comment.
any reason for changing the variable names?
|
please see #724 |
Summary by CodeRabbit