Modify post_fp_cp2k, adapt to SCF not converged situation. - #1562
Modify post_fp_cp2k, adapt to SCF not converged situation.#1562dulinhan wants to merge 6 commits into
post_fp_cp2k, adapt to SCF not converged situation.#1562Conversation
… to run different system at one single iteration model_devi step.
|
Warning Review limit reachedNext included review available in 37 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes to the Changes
Sequence Diagram(s)Old FlowsequenceDiagram
participant Client
participant post_fp_cp2k
Client->>+post_fp_cp2k: Call function with iter_index, jdata, rfailed
post_fp_cp2k->>post_fp_cp2k: Initialize all_sys
loop sys_output
post_fp_cp2k->>post_fp_cp2k: Process system
end
post_fp_cp2k->>-Client: Return result
New FlowsequenceDiagram
participant Client
participant post_fp_cp2k
participant LogFile
Client->>+post_fp_cp2k: Call function with iter_index, jdata, rfailed
post_fp_cp2k->>post_fp_cp2k: Initialize all_sys
loop sys_output
post_fp_cp2k->>post_fp_cp2k: Read file content
alt SCF run NOT converged
post_fp_cp2k->>LogFile: Write log entry
post_fp_cp2k->>post_fp_cp2k: Skip file
else
post_fp_cp2k->>post_fp_cp2k: Process system
post_fp_cp2k->>post_fp_cp2k: Append system to all_sys
post_fp_cp2k->>post_fp_cp2k: Increment count
end
end
post_fp_cp2k->>-Client: Return result
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1562 +/- ##
==========================================
+ Coverage 49.59% 49.81% +0.22%
==========================================
Files 83 83
Lines 14844 14993 +149
==========================================
+ Hits 7362 7469 +107
- Misses 7482 7524 +42 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| log_file_path = os.path.join(work_path, f"{ss}.fp-fail.log") | ||
| all_sys = dpdata.MultiSystems(type_map=jdata["type_map"]) | ||
| for oo in sys_output: | ||
| _sys = dpdata.LabeledSystem(oo, fmt="cp2k/output") |
There was a problem hiding this comment.
why does this fmt revert back?
Lines 4483 to 4489 in 54e48c6
| all_sys.append(_sys) | ||
| with open(oo, 'r') as file: | ||
| content = file.read() | ||
| if 'SCF run NOT converged' in content: |
There was a problem hiding this comment.
cp2kdata already checks the convergence.
if the scf is not converged, dpdata will return empty object.
https://github.com/robinzyb/cp2kdata/blob/d74c14cbf7470451af443c706d8479c8b486a68d/cp2kdata/dpdata_plugin.py#L24-L43
There was a problem hiding this comment.
Yes, it will. But when using MultiSystems, the empty object will be added into that, for example create an empty "C0H0O0". This will raise error when save the MultiSystems to data.xxx. Maybe we should change the subroutine which you mentioned to skip instead of creating empty object.
| with open(oo) as file: | ||
| content = file.read() | ||
| if "SCF run NOT converged" in content: | ||
| with open(log_file_path, "a") as log_file: | ||
| log_file.write(f"Skipping file {oo} due to SCF run NOT converged\n") | ||
| continue |
There was a problem hiding this comment.
move these lines after _sys=dpdata.LabeledSystem
| with open(oo) as file: | |
| content = file.read() | |
| if "SCF run NOT converged" in content: | |
| with open(log_file_path, "a") as log_file: | |
| log_file.write(f"Skipping file {oo} due to SCF run NOT converged\n") | |
| continue | |
| if len(_sys) == 0: | |
| with open(log_file_path, "a") as log_file: | |
| log_file.write(f"Skipping file {oo} due to SCF run NOT converged\n") | |
| continue |
| if "SCF run NOT converged" in content: | ||
| with open(log_file_path, "a") as log_file: | ||
| log_file.write(f"Skipping file {oo} due to SCF run NOT converged\n") | ||
| continue |
There was a problem hiding this comment.
Move SCF check and logging after _sys instantiation
To ensure _sys is only instantiated if SCF converges, move these lines after _sys=dpdata.LabeledSystem.
- if "SCF run NOT converged" in content:
- with open(log_file_path, "a") as log_file:
- log_file.write(f"Skipping file {oo} due to SCF run NOT converged\n")
- continue
_sys = dpdata.LabeledSystem(
oo, fmt="cp2kdata/e_f", type_map=jdata["type_map"]
)
+ if "SCF run NOT converged" in content:
+ with open(log_file_path, "a") as log_file:
+ log_file.write(f"Skipping file {oo} due to SCF run NOT converged\n")
+ continueCommittable 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.
| if "SCF run NOT converged" in content: | |
| with open(log_file_path, "a") as log_file: | |
| log_file.write(f"Skipping file {oo} due to SCF run NOT converged\n") | |
| continue | |
| _sys = dpdata.LabeledSystem( | |
| oo, fmt="cp2kdata/e_f", type_map=jdata["type_map"] | |
| ) | |
| if "SCF run NOT converged" in content: | |
| with open(log_file_path, "a") as log_file: | |
| log_file.write(f"Skipping file {oo} due to SCF run NOT converged\n") | |
| continue |
There was a problem hiding this comment.
Verdict: Changes requested. Successful tasks are counted twice, which can make the failure ratio negative and bypass ratio_failed. Matching one SCF message is also insufficient to establish that dpdata parsed a valid frame. Current master already has a more reliable empty-LabeledSystem check; a smaller logging-only PR should be based on that implementation.
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
| all_sys.append(_sys) | ||
| icount += 1 | ||
|
|
||
| icount += len(all_sys) |
There was a problem hiding this comment.
[Blocking] icount is already incremented once for each successful output in the inner loop, and this line adds len(all_sys) again, counting every successful task twice. For example, six successes out of ten tasks produce icount=12 and a failure ratio of -20%, bypassing ratio_failed. Remove this increment and append/count only when len(_sys) > 0. Other parse failures can also produce an empty LabeledSystem, so checking only the literal SCF run NOT converged message is insufficient.
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Independent review C
REQUEST_CHANGES. The new non-converged-output filtering is useful, but the success counter is incremented twice for every accepted CP2K result, corrupting the completion/failure accounting.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
| all_sys.append(_sys) | ||
| icount += 1 | ||
|
|
||
| icount += len(all_sys) |
There was a problem hiding this comment.
icount is already incremented for every system appended to all_sys in the loop above. Adding len(all_sys) here double-counts every successful output, so the later icount / tcount accounting can exceed 1 or mask skipped/non-converged tasks. Remove this second increment.
| icount += len(all_sys) |
There was a problem hiding this comment.
Fixed in e3e6094 by removing the second success increment; each converged CP2K output is now counted once. Validation: Python syntax compilation passed. The historical CP2K unittest cannot import in the current environment because modern dpdata no longer exposes the dpdata.abacus API required by this old branch.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Independent review B\n\nRequesting changes because successful CP2K results are counted twice, corrupting the failure-ratio calculation.\n\nCoding agent: Codex\nCodex version: codex-cli 0.151.0\nModel: gpt-5.6-sol\nReasoning effort: xhigh
| all_sys.append(_sys) | ||
| icount += 1 | ||
|
|
||
| icount += len(all_sys) |
There was a problem hiding this comment.
Each converged output already increments icount at line 4513. Adding len(all_sys) again doubles every successful frame, which can make (tcount - icount) / tcount negative and suppress the configured failure threshold. Remove this second increment.\n\nsuggestion\n
There was a problem hiding this comment.
Fixed in e3e6094 by removing the second success increment; each converged CP2K output is now counted once. Validation: Python syntax compilation passed. The historical CP2K unittest cannot import in the current environment because modern dpdata no longer exposes the dpdata.abacus API required by this old branch.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Independent review A
Blocking finding attached inline. The new failure accounting double-counts successful CP2K results and can turn the reported failed fraction negative.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
| all_sys.append(_sys) | ||
| icount += 1 | ||
|
|
||
| icount += len(all_sys) |
There was a problem hiding this comment.
P1: Remove this second success increment. icount is already incremented once for every parsed output inside the loop above; adding len(all_sys) again counts the same successful tasks a second time. With one converged output, for example, tcount == 1 and icount == 2, producing a negative failure ratio and disabling the intended failure threshold.
| icount += len(all_sys) |
There was a problem hiding this comment.
Fixed in e3e6094 by removing the second success increment; each converged CP2K output is now counted once. Validation: Python syntax compilation passed. The historical CP2K unittest cannot import in the current environment because modern dpdata no longer exposes the dpdata.abacus API required by this old branch.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
Coding-Agent: Codex Codex-Version: codex-cli 0.151.0 Model: gpt-5.6-sol Reasoning-Effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
The CP2K success double-count was removed in e3e6094, so every converged output contributes exactly once. The branch is mergeable and its current checks pass.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Independent re-review C
No blocking issues found; converged CP2K outputs now increment the accepted-frame count exactly once.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Independent re-review B
Reviewed the current merge result against the maintained CP2K post-processing path. The added convergence guard skips explicitly non-converged outputs before parsing, logs the skipped path, and preserves the existing empty-system and failure-ratio accounting. I found no blocking issue and approve this head.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Independent re-review A
Approved at e3e6094. I checked the CP2K non-convergence detection, success/failure accounting, emitted datasets, and passing CI.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
4 participants
Check whether "SCF run NOT converged" appears before reading the output file. Print the path of unfinished calculation to the log file. Make the
icountcounter work effectivelySummary by CodeRabbit