fix #359: read atom names from POTCAR if not present in OUTCAR - #443
fix #359: read atom names from POTCAR if not present in OUTCAR#443hoba87 wants to merge 4 commits into
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## devel #443 +/- ##
==========================================
- Coverage 82.53% 82.52% -0.01%
==========================================
Files 68 68
Lines 6205 6210 +5
==========================================
+ Hits 5121 5125 +4
- Misses 1084 1085 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| atom_names.append(_ii.split("_")[0]) | ||
| else: | ||
| atom_names.append(_ii) | ||
| elif "POTCAR" in ii: |
There was a problem hiding this comment.
my outcar reads
INCAR:
POTCAR: PAW_PBE Mg 13Apr2007
POTCAR: PAW_PBE Mg 13Apr2007
VRHFIN =Mg: s2p0
LEXCH = PE
EATOM = 23.0369 eV, 1.6932 Ry
TITEL = PAW_PBE Mg 13Apr2007
LULTRA = F use ultrasoft PP ?
IUNSCR = 1 unscreen: 0-lin 1-nonlin 2-no
Then the same atom name will be added three times.
There was a problem hiding this comment.
The new commit don't allow duplicated atom names
There was a problem hiding this comment.
The dpdata is expected to abstract atom names in an order that is exact the same as how the potcars are concatenated.
Your implementation does not work for the cases like "A" "B" "A" "B"
| atom_names.append(_ii.split("_")[0]) | ||
| else: | ||
| atom_names.append(_ii) | ||
| elif "POTCAR" in ii: |
There was a problem hiding this comment.
The dpdata is expected to abstract atom names in an order that is exact the same as how the potcars are concatenated.
Your implementation does not work for the cases like "A" "B" "A" "B"
|
Why does it not work for "A" "B" "A" "B", just skipping the second "A" "B"? |
The expected atom names is "A" "B" "A" "B". Your PR changes the behavior of dpdata. |
|
Ok, do I understand you right, there exist cases with different pseudopotentials for the same atom type? If that is the case, I have no idea how to cover that. |
The same pseudopotential file for difference sections of atoms. One is allowed to generate POTCAR by e.g. and write four sections of atoms having types A B A B in POSCAR. |
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Requesting changes because the fallback deduplication is not structurally safe, and the branch conflicts with master. The requirement has already been implemented by #844 / bb14c5a with ordered repeated-half validation and tests, so closing this PR is preferable to rebasing this version.
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
| atom_names.append(_ii) | ||
| # for case like : TITEL = PAW_PBE Sn_d 06Sep2000 | ||
| atom_name = ii.split()[3].split("_")[0] | ||
| if atom_name not in atom_names: |
There was a problem hiding this comment.
[P1] Preserve POTCAR record structure instead of globally deduplicating names
OUTCAR may print POTCAR records in repeated halves such as A B A B. Global membership deduplication collapses this to A B without verifying the repeated structure; with four ions-per-type entries the result has two names, four counts, and atom types 0 through 3. Current master instead preserves order and removes the second half only after validating that both halves match. This implementation should not be merged.
3 participants
No description provided.