Conversation
… 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 |
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
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
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
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
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
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
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
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
njzjz-bot
left a comment
There was a problem hiding this comment.
Reviewed commit e3e609429f3271b955b64b845752af3c0df8d109.
Reviewed the explicit SCF non-convergence filter, failure log, and success counter. The earlier request to skip empty parsed systems is still relevant: the parser may return zero frames for other failed outputs, but icount is incremented unconditionally. The explicit warning-string path is handled; the broader empty-result case remains unaddressed.
Existing inline discussion: #1562 (comment)
Validation limitation: direct import of this historical head is blocked by its dependency on the removed dpdata.abacus API in the current isolated environment. The review of this path is static; no CP2K calculation was run.
Coding agent: Codex
Codex version: codex-cli 0.154.0
Model: gpt-6-astra
Reasoning effort: xhigh
njzjz-bot
left a comment
There was a problem hiding this comment.
There is still one high-confidence blocking correctness issue on this head, already captured in the existing unresolved inline thread at the current _sys handling, so I am not duplicating that inline comment.
cp2kdata already reports unsuccessful/non-converged parses by returning an empty LabeledSystem, and the discussion confirms that appending such an empty system can create an empty C0H0O0 entry and fail when MultiSystems is written. The current implementation only pre-scans for the exact literal "SCF run NOT converged", then unconditionally appends _sys and increments icount. Any case the parser classifies as empty but that does not contain that exact literal is therefore still counted as successful and may still poison all_sys. The robust fix is the one already suggested in the unresolved thread: instantiate _sys, check len(_sys) == 0, log/skip it, and only append/increment for non-empty systems. A regression test should cover the empty-parser-result path, not only the literal-string path.
The previous duplicate icount += len(all_sys) defect is fixed in e3e6094. Exact-head GitHub Actions/Codecov checks that ran are green, but the PR is currently not mergeable against master; that conflict will also need resolution before merge.
Agent: ChatGPT
Model: GPT-5.6 Sol
GitHub account: njzjz-bot
Reviewed head: e3e6094
Trigger: scheduled all-PR monitoring
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