Enhance poscar_scale function (/data/gen.py)to detect Selective dynamics - #1439
Enhance poscar_scale function (/data/gen.py)to detect Selective dynamics#1439Mancn-Xu wants to merge 2 commits into
Conversation
Mancn-Xu
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1439 +/- ##
==========================================
+ Coverage 48.97% 49.42% +0.44%
==========================================
Files 83 83
Lines 14792 14820 +28
==========================================
+ Hits 7245 7325 +80
+ Misses 7547 7495 -52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@CodiumAI-Agent /review --pr_reviewer.inline_code_comments=true |
|
@CodiumAI-Agent /improve --extend |
PR Analysis
PR Feedback💡 General suggestions: The PR is well-structured and the changes are clear. It would be beneficial to add tests to ensure that the new functionality works as expected. Also, consider handling the case where the "Selective dynamics" line is not directly above the coordinate type line, even though it's not the standard format. ✨ Usage guide:Overview: With a configuration file, use the following template:
See the review usage page for a comprehensive guide on using this tool. |
| if "D" == lines[coord_type_line][0] or "d" == lines[coord_type_line][0]: | ||
| lines = poscar_scale_direct(lines, scale) | ||
| elif "C" == lines[7][0] or "c" == lines[7][0]: | ||
| elif "C" == lines[coord_type_line][0] or "c" == lines[coord_type_line][0]: | ||
| lines = poscar_scale_cartesian(lines, scale) |
There was a problem hiding this comment.
Suggestion: Use the str.startswith() method to check the starting character of a string in a case-insensitive manner, instead of checking equality with both the uppercase and lowercase versions of the character. [best practice]
| if "D" == lines[coord_type_line][0] or "d" == lines[coord_type_line][0]: | |
| lines = poscar_scale_direct(lines, scale) | |
| elif "C" == lines[7][0] or "c" == lines[7][0]: | |
| elif "C" == lines[coord_type_line][0] or "c" == lines[coord_type_line][0]: | |
| lines = poscar_scale_cartesian(lines, scale) | |
| if lines[coord_type_line].startswith(("D", "d")): | |
| lines = poscar_scale_direct(lines, scale) | |
| elif lines[coord_type_line].startswith(("C", "c")): | |
| lines = poscar_scale_cartesian(lines, scale) |
| raise RuntimeError( | ||
| f"Unknown poscar style at line {coord_type_line + 1}: {lines[coord_type_line]}" | ||
| ) |
There was a problem hiding this comment.
Suggestion: The error message in the RuntimeError could be more informative by specifying that the unknown style is related to the coordinate type. [enhancement]
| raise RuntimeError( | |
| f"Unknown poscar style at line {coord_type_line + 1}: {lines[coord_type_line]}" | |
| ) | |
| raise RuntimeError( | |
| f"Unknown coordinate type at line {coord_type_line + 1}: {lines[coord_type_line]}" | |
| ) |
There was a problem hiding this comment.
So, is it poscar style or coordinate type?
There was a problem hiding this comment.
Verdict: Changes requested. The new logic locates the coordinate-type line after Selective dynamics, but the Cartesian scaler still reads from the old fixed index and attempts to convert T/F flags to floats. A standard Selective+Cartesian POSCAR will fail. The PR also conflicts with master and lacks tests for the four format combinations.
Note: The Codex quota is about to reset, so I am using the remaining tokens to review all open PRs in this repository.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| lines = poscar_scale_direct(lines, scale) | ||
| elif "C" == lines[7][0] or "c" == lines[7][0]: | ||
| elif "C" == lines[coord_type_line][0] or "c" == lines[coord_type_line][0]: | ||
| lines = poscar_scale_cartesian(lines, scale) |
There was a problem hiding this comment.
[Blocking] After detecting Selective+Cartesian format, this still calls the existing poscar_scale_cartesian(), which starts at the fixed old index. In a standard file that line contains Cartesian, while coordinates begin on the following line, so the first float() conversion fails. Even after correcting the start index, the scaler currently tries to convert the trailing T/F selective flags to floats. Pass the computed coordinate start into the scaler, scale only the first three fields, preserve the remaining flags, and test Direct/Cartesian with and without Selective dynamics.
5 participants
Summary
This pull request updates the
poscar_scalefunction in thegen.pyfile to enhance its functionality by accurately detecting the presence of "Selective dynamics" in POSCAR files. The update ensures that the coordinate type determination is correctly handled based on whether the "Selective dynamics" line is present, improving the function's reliability and correctness in handling various POSCAR file configurations.Changes Made
Impact
This enhancement allows users to work with a wider variety of POSCAR files, especially those that include the "Selective dynamics" setting, without manually adjusting the script for different file formats. It ensures that the
poscar_scalefunction is more adaptable and accurate, providing a robust solution for scaling atomic positions in POSCAR files.Additional Notes
This update assumes that the "Selective dynamics" line, if present, is directly above the coordinate type line in POSCAR files, which is the standard format. Users should ensure that their POSCAR files follow this format for the script to function correctly.