Adjust pp_file as an unnessary parameter in save to abacus/stru #752
Conversation
CodSpeed Performance ReportMerging #752 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #752 +/- ##
=======================================
Coverage 85.13% 85.14%
=======================================
Files 81 81
Lines 7516 7520 +4
=======================================
+ Hits 6399 6403 +4
Misses 1117 1117 ☔ View full report in Codecov by Sentry. |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
dpdata/abacus/scf.py(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
dpdata/abacus/scf.py
[warning] 767-767: dpdata/abacus/scf.py#L767
Added line #L767 was not covered by tests
[warning] 770-770: dpdata/abacus/scf.py#L770
Added line #L770 was not covered by tests
🪛 Ruff
dpdata/abacus/scf.py
767-767: No explicit stacklevel keyword argument found
(B028)
🔇 Additional comments (3)
dpdata/abacus/scf.py (3)
638-638: LGTM: Function signature change aligns with requirements.
Making pp_file optional with a default value of None aligns with the requirement to support saving as stru format without pseudopotential information.
782-788: LGTM: Proper handling of empty pp_file strings.
The implementation correctly handles both empty and non-empty pp_file strings while maintaining the link_file functionality.
638-638: Verify impact of optional pp_file parameter.
Let's verify that making pp_file optional doesn't break existing code that might assume its presence.
✅ Verification successful
Making pp_file optional is safe and properly handled
The code already has proper handling for when pp_file is None:
- There's explicit documentation indicating pp_file is optional
- A warning message is logged when pp_file is not provided
- The function continues execution with empty strings for pseudo potential files
- The only caller (in dpdata/plugins/abacus.py) doesn't explicitly pass pp_file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to make_unlabeled_stru to verify pp_file usage patterns
rg -A 2 "make_unlabeled_stru\("
# Search for imports of make_unlabeled_stru to check usage in other files
rg -l "from.*make_unlabeled_stru import|import.*make_unlabeled_stru"
Length of output: 412
Script:
#!/bin/bash
# Let's check the implementation and usage context of make_unlabeled_stru
ast-grep --pattern 'def make_unlabeled_stru($$$)'
# Check how pp_file is used within the function
rg -B 5 -A 10 "pp_file=None" dpdata/abacus/scf.py
# Check for any other references to pp_file in the codebase
rg "pp_file" dpdata/abacus/
Length of output: 1159
| if pp_file is not None: | ||
| ppfiles = process_file_input( | ||
| ndarray2list(pp_file), data["atom_names"], "pp_file" | ||
| ) | ||
| else: | ||
| warnings.warn( | ||
| "pp_file is not provided, will use empty string for pseudo potential file." | ||
| ) | ||
| ppfiles = [""] * len(data["atom_names"]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance warning implementation and add test coverage.
- Add stacklevel to the warning to help trace its origin
- Add test coverage for the None case
- warnings.warn(
- "pp_file is not provided, will use empty string for pseudo potential file."
- )
+ warnings.warn(
+ "pp_file is not provided, will use empty string for pseudo potential file.",
+ stacklevel=2
+ )Would you like me to help create test cases for the scenario where pp_file is None?
📝 Committable 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 pp_file is not None: | |
| ppfiles = process_file_input( | |
| ndarray2list(pp_file), data["atom_names"], "pp_file" | |
| ) | |
| else: | |
| warnings.warn( | |
| "pp_file is not provided, will use empty string for pseudo potential file." | |
| ) | |
| ppfiles = [""] * len(data["atom_names"]) | |
| if pp_file is not None: | |
| ppfiles = process_file_input( | |
| ndarray2list(pp_file), data["atom_names"], "pp_file" | |
| ) | |
| else: | |
| warnings.warn( | |
| "pp_file is not provided, will use empty string for pseudo potential file.", | |
| stacklevel=2 | |
| ) | |
| ppfiles = [""] * len(data["atom_names"]) |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 767-767: dpdata/abacus/scf.py#L767
Added line #L767 was not covered by tests
[warning] 770-770: dpdata/abacus/scf.py#L770
Added line #L770 was not covered by tests
🪛 Ruff
767-767: No explicit stacklevel keyword argument found
(B028)
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/test_abacus_stru_dump.py (1)
32-34: Enhance test coverage for optional pp_file scenarios.The basic test verifies file creation without pp_file, which aligns with the PR objective. Consider enhancing the test coverage by:
- Verifying the content of the generated STRU_tmp file
- Asserting warning messages about missing pp_file
- Testing combinations with other optional parameters
Example enhancement:
def test_dump_stru_without_pporb(self): self.system_ch4.to("stru", "STRU_tmp", mass=[12, 1]) self.assertTrue(os.path.isfile("STRU_tmp")) + # Verify file content + with open("STRU_tmp") as f: + content = f.read() + # Verify expected structure without pp_file entries + self.assertNotIn("PP_FILE", content) + # Verify basic structure is maintained + self.assertIn("ATOMIC_SPECIES", content) + + # Test with warnings + with self.assertWarns(UserWarning): + self.system_ch4.to("stru", "STRU_tmp", mass=[12, 1])
In case, some one may just save only as a stru file and has no pseudopotential information.
Summary by CodeRabbit
Bug Fixes
New Features
Tests