fix bug in reading abacus stru #1714
Conversation
📝 WalkthroughWalkthroughThe pull request updates the module organization and refactors structure creation logic. Import statements in several files have been modified to reference new modules within the Changes
Sequence Diagram(s)sequenceDiagram
participant Run as dpgen/generator/run.py
participant Gen as dpgen/generator/lib/abacus_scf.py
participant StruUtil as dpdata.abacus.stru
participant Test as tests/auto_test/test_abacus.py
Run->>Gen: make_fp_abacus_scf(iter_index, jdata)
Gen->>StruUtil: make_unlabeled_stru(sys_data, fp_pp_files, fp_orb_files, fp_dpks_descriptor)
StruUtil-->>Gen: Return structure data
Gen->>Run: Return processed structure
Note over Gen,StruUtil: get_abacus_STRU now uses get_frame_from_stru
Test->>Gen: test_compute()
Gen-->>Test: Return filtered computed results
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (6)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dpgen/generator/lib/abacus_scf.py (1)
216-225: Unused 'type_map' parameter
Thetype_mapparameter is never used in the function body, potentially indicating a leftover from previous refactoring. Consider removing it to reduce confusion.- def make_abacus_scf_stru(sys_data, fp_pp_files, fp_orb_files=None, fp_dpks_descriptor=None, type_map=None, pporb=""): + def make_abacus_scf_stru(sys_data, fp_pp_files, fp_orb_files=None, fp_dpks_descriptor=None, pporb=""):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dpgen/auto_test/lib/abacus.py(1 hunks)dpgen/generator/lib/abacus_scf.py(3 hunks)dpgen/generator/run.py(0 hunks)tests/auto_test/test_abacus.py(2 hunks)
💤 Files with no reviewable changes (1)
- dpgen/generator/run.py
✅ Files skipped from review due to trivial changes (1)
- dpgen/auto_test/lib/abacus.py
🔇 Additional comments (4)
dpgen/generator/lib/abacus_scf.py (2)
6-7: Add import from dpdata.abacus.stru
The addition ofget_frame_from_struandmake_unlabeled_struis consistent with the updated dpdata interface. This should streamline the handling of ABACUS structures.
243-250: Potential indexing assumption
Accessingdata["cells"][0]anddata["coords"][0]assumes only a single frame is returned byget_frame_from_stru. If a multi-frame STRU is provided, this will break. Verify that only a single frame is used.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if multi-frame usage of get_abacus_STRU or get_frame_from_stru is present in the repo rg "get_abacus_STRU|get_frame_from_stru" -A 5Length of output: 11652
Subject: Verification of Single-Frame Assumption in get_frame_from_stru Usage
I reviewed the repository search results. All calls to
get_abacus_STRU(which internally callsget_frame_from_stru) pass a STRU file that is expected to contain a single frame (e.g., in tests and auto-tests). There’s no evidence in the repo of multi-frame STRU usage or handling. This indicates that the current implementation of indexing[0]is based on the expected input.tests/auto_test/test_abacus.py (2)
219-219: Renamed test method
Renaming totest_computeimproves clarity and correctness.
249-252: Refined comparison approach
Filtering only relevant keys for comparison is a good practice for focusing the test and avoiding false positives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dpgen/generator/lib/abacus_scf.py (1)
244-255: LGTM! Function updated to use the new dpdata interface.The changes effectively utilize the new
get_frame_from_strufunction and handle the data structure transformation well. The code gracefully handles optional data likeorb_filesanddpks_descriptor.Consider adding type hints to improve code maintainability:
-def get_abacus_STRU(STRU, INPUT=None, n_ele=None): +def get_abacus_STRU(STRU: str, INPUT: str | None = None, n_ele: int | None = None) -> dict:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dpgen/auto_test/lib/abacus.py(1 hunks)dpgen/generator/lib/abacus_scf.py(3 hunks)dpgen/generator/run.py(0 hunks)tests/auto_test/test_abacus.py(2 hunks)
💤 Files with no reviewable changes (1)
- dpgen/generator/run.py
🚧 Files skipped from review as they are similar to previous changes (2)
- dpgen/auto_test/lib/abacus.py
- tests/auto_test/test_abacus.py
🔇 Additional comments (3)
dpgen/generator/lib/abacus_scf.py (3)
6-6: LGTM! Import changes align with PR objectives.The updated imports utilize the new general interface from dpdata/abacus, which is consistent with the PR's goal of addressing the dpdata library update.
206-229: LGTM! Function refactored to use the new dpdata interface.The refactoring simplifies the code by:
- Delegating structure creation to
make_unlabeled_stru- Focusing on path construction
- Removing redundant code
This change aligns well with the PR objectives of utilizing the more general dpdata interface.
6-6: Verify dpdata dependency version.Let's ensure the required dpdata version with PR #793 is available.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check installed dpdata version and available versions pip show dpdata 2>/dev/null || echo "dpdata not installed" pip index versions dpdata 2>/dev/null || echo "Failed to fetch versions"Length of output: 490
dpdata Dependency Version Verified
- The verification script confirms that version 0.2.22 of dpdata—which incorporates the changes from PR An error occurred while running task08.fp(vasp) #793—is available from PyPI.
- Although dpdata is not installed in the current environment, the available versions list shows that the required dependency is indeed published.
|
Hi @pxlxingliang, the dpdata has bumped to 0.2.23; the CI hasn't passed. |
Got, I will check it. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #1714 +/- ##
==========================================
- Coverage 49.51% 49.40% -0.11%
==========================================
Files 83 83
Lines 14863 14770 -93
==========================================
- Hits 7359 7297 -62
+ Misses 7504 7473 -31 ☔ View full report in Codecov by Sentry. |
|
Hi, @njzjz, the bug is fixed now, please review it again. |
|
Hi @pxlxingliang, please bump the minimal dpdata version. |
Fix the bug induced by the update of dpdata.
Now the reading and writing of ABACUS STRU is by a more general dpdata/abacus interface, and remove some redundant codes.
This commit is based on dpdata commit deepmodeling/dpdata#793, and it is valid after that commit is merged.
related issue:
#1711
Summary by CodeRabbit