add new format : qe/pw/md - #472
Conversation
Signed-off-by: pee8379 <pee8379@users.noreply.github.com>
Signed-off-by: pee8379 <pee8379@users.noreply.github.com>
for more information, see https://pre-commit.ci
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #472 +/- ##
==========================================
- Coverage 82.76% 80.82% -1.94%
==========================================
Files 70 71 +1
Lines 6313 6494 +181
==========================================
+ Hits 5225 5249 +24
- Misses 1088 1245 +157
☔ View full report in Codecov by Sentry. |
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Could you please add unittests for your PR? Thank you.
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Requesting changes because this parser can silently return wrong geometry and inconsistent frame arrays for valid QE trajectories. The four inline findings need a coordinated rewrite against the current format backend plus focused unit tests; the branch also conflicts with master.
Process note: the Codex usage allowance is about to reset, so I am spending the remaining token budget now on this review.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| blk = get_block(newlines, "ATOMIC_POSITIONS") | ||
| blk = blk[0 : sum(natoms)] | ||
| for ii in blk: | ||
| ret.append([float(jj) for jj in ii.split()[1:4]]) |
There was a problem hiding this comment.
[P1] Convert ATOMIC_POSITIONS according to the card unit
This appends the three raw numbers for every ATOMIC_POSITIONS card. Valid crystal coordinates therefore remain fractional, while bohr or alat values are treated as angstrom, silently corrupting coordinates. Parse the card option for each frame and convert crystal through that frame cell and bohr or alat through the appropriate length scale.
| newlines = lines[i:] | ||
| blk = get_block(newlines, "CELL_PARAMETERS") | ||
| for ii in blk: | ||
| ret.append([float(jj) for jj in ii.split()[0:3]]) |
There was a problem hiding this comment.
[P1] Convert variable CELL_PARAMETERS according to its declared unit
The raw lattice vectors are appended without inspecting angstrom, bohr, or alat. A vc-md output using CELL_PARAMETERS (bohr) is therefore returned in angstrom numerically, and the derived virial volume is wrong as well. Unit handling must be explicit for every cell frame.
|
|
||
| atom_names, atom_numbs, atom_types = get_atoms(inlines) | ||
|
|
||
| coords = get_coords(outlines, atom_numbs) |
There was a problem hiding this comment.
[P1] Apply begin and step to every frame-aligned array
begin and step are public arguments, but coords, energies, forces, stresses, and cells are all built from the complete output here and are never sliced. A caller requesting a subset still receives every frame. Apply one consistent selected-frame index to all arrays and test non-default begin and step.
| calculation = calculation.lower() | ||
| if calculation == "md" or calculation == "relax": | ||
| cells = get_cell(inlines) | ||
| cells = np.tile(cells, (len(virials), 1, 1)) |
There was a problem hiding this comment.
[P1] Do not size cells from an optional stress array
For an ordinary md or relax output without a stress block, virials is empty, so this creates zero cell frames while coordinates, energies, and forces remain non-empty. Cells must follow the coordinate frame count; omit virials when stress is unavailable instead of making the entire LabeledSystem internally inconsistent.
3 participants
allow dpdata accept several new calculation modes of quantum espresso pw; md, vc-md, relax, vc-relax,
new format key 'qe/pw/md' is used.
many of codes are derived from original scf.py file.