Fix: fparam to mixed #832
Fix: fparam to mixed #832
Conversation
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Updated developer tooling configurations to use a newer linting hook version. - **Refactor** - Streamlined structure processing so that magnetic data is included only when defined, resulting in cleaner output. - Optimized coordinate adjustments with vectorized operations for improved performance. - **Tests** - Cleaned up test data by removing redundant magnetic moment fields from atomic position entries. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
CodSpeed Performance ReportMerging #832 will not alter performanceComparing Summary
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #832 +/- ##
==========================================
+ Coverage 85.33% 85.36% +0.02%
==========================================
Files 82 82
Lines 7515 7528 +13
==========================================
+ Hits 6413 6426 +13
Misses 1102 1102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant System (dpdata)
participant FileSystem
User->>System (dpdata): Save MultiSystem with fparam/aparam
System (dpdata)->>FileSystem: Write set.XXX/fparam.npy, aparam.npy
User->>System (dpdata): Load MultiSystem from DeepMD mixed format
System (dpdata)->>FileSystem: Read set.XXX/fparam.npy, aparam.npy
System (dpdata)->>System (dpdata): Partition fparam/aparam per system
System (dpdata)-->>User: Return MultiSystem with correct fparam/aparam shapes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/benchmark.yml(1 hunks).github/workflows/test.yml(1 hunks).pre-commit-config.yaml(1 hunks)dpdata/abacus/stru.py(2 hunks)dpdata/deepmd/mixed.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
dpdata/deepmd/mixed.py
[warning] 62-63: dpdata/deepmd/mixed.py#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 65-66: dpdata/deepmd/mixed.py#L65-L66
Added lines #L65 - L66 were not covered by tests
🔇 Additional comments (6)
.github/workflows/benchmark.yml (1)
16-16: Upgradeastral-sh/setup-uvto v6
Bumping the UV action to v6 aligns this workflow with the test pipeline and unlocks caching improvements. Please verify that v6 still supports the same inputs (enable-cache,cache-dependency-glob)..pre-commit-config.yaml (1)
24-24: Updateruffpre-commit hook to v0.11.11
This patch release may include new rules or fixes—rerun the full pre-commit suite to catch any unexpected lint diffs..github/workflows/test.yml (1)
21-21: Syncastral-sh/setup-uvaction to v6
Keeping the setup-uv action version consistent across workflows; confirm that caching parameters (enable-cache,cache-dependency-glob,cache-suffix) are still supported in v6.dpdata/abacus/stru.py (2)
317-317: LGTM! Good typo fix.The function name correction from
get_carteisan_coordstoget_cartesian_coordsimproves code readability and correctness.
381-381: LGTM! Consistent function call update.The function call was correctly updated to match the fixed function name.
dpdata/deepmd/mixed.py (1)
30-31: LGTM! Proper handling of optional fields.The use of
data.get()with defaultNoneis the correct approach for handling optional fields.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_deepmd_mixed.py (3)
473-474: Consider using more realistic test data dimensions.The random arrays for fparam and aparam use arbitrary dimensions (2 and 3 respectively). Consider using dimensions that reflect realistic use cases or documenting why these specific dimensions were chosen.
- tmp_data["fparam"] = np.random.random([nframes, 2]) - tmp_data["aparam"] = np.random.random([nframes, natoms, 3]) + # Using common fparam dimension (e.g., temperature, pressure) + tmp_data["fparam"] = np.random.random([nframes, 2]) + # Using common aparam dimension (e.g., atomic charge, mass, polarizability) + tmp_data["aparam"] = np.random.random([nframes, natoms, 3])
458-458: Add class docstring for better documentation.Static analysis correctly identifies missing docstring. Consider adding a brief description of what this test class validates.
class TestMixedSystemWithFparamAparam(unittest.TestCase, CompLabeledSys, IsNoPBC): + """Test persistence of fparam and aparam fields in DeepMD mixed format.""" def setUp(self):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 458-458: Missing class docstring
(C0115)
487-494: Add method docstrings for test clarity.Both test methods would benefit from brief docstrings explaining their purpose.
def test_fparam_exists(self): + """Test that fparam field is preserved during save/load cycle.""" self.assertTrue("fparam" in self.system_1.data) def test_aparam_exists(self): + """Test that aparam field is preserved during save/load cycle.""" self.assertTrue("aparam" in self.system_1.data)Also applies to: 496-503
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 487-487: Missing function or method docstring
(C0116)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dpdata/deepmd/mixed.py(2 hunks)tests/test_deepmd_mixed.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dpdata/deepmd/mixed.py
🧰 Additional context used
🪛 Pylint (3.3.7)
tests/test_deepmd_mixed.py
[convention] 458-458: Missing class docstring
(C0115)
[convention] 487-487: Missing function or method docstring
(C0116)
[convention] 496-496: Missing function or method docstring
(C0116)
🪛 GitHub Actions: Python package
tests/test_deepmd_mixed.py
[error] 479-479: NotImplementedError: DeePMDMixedFormat doesn't support LabeledSystem.from - raised in multiple tests during setup when initializing dpdata.LabeledSystem
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_deepmd_mixed.py (1)
526-538: Critical issue: Test will fail due to missing loading support.Based on the past review comments, the pipeline fails with
NotImplementedError: DeePMDMixedFormat doesn't support LabeledSystem.fromwhen trying to load from the mixed format. This test cannot pass until the underlying implementation is fixed.The test logic itself is well-structured, but it requires the implementation of loading support in the mixed format handler before it can run successfully.
🧹 Nitpick comments (2)
tests/test_deepmd_mixed.py (2)
463-465: Add class docstring per coding standards.The test class is missing a docstring explaining its purpose.
+class TestMixedSystemWithFparamAparam( + unittest.TestCase, CompLabeledMultiSys, MultiSystems, MSAllIsNoPBC +): + """Test mixed DeepMD format with optional fparam and aparam fields."""🧰 Tools
🪛 Pylint (3.3.7)
[convention] 463-463: Missing class docstring
(C0115)
[refactor] 463-463: Too many instance attributes (12/7)
(R0902)
557-561: Add method docstring.The test method is missing a docstring explaining what it tests.
+def test_len(self): + """Test that the number of systems is correctly preserved."""🧰 Tools
🪛 Pylint (3.3.7)
[convention] 557-557: Missing function or method docstring
(C0116)
[convention] 561-561: Missing function or method docstring
(C0116)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_deepmd_mixed.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_deepmd_mixed.py (3)
dpdata/data_type.py (2)
Axis(15-21)DataType(35-131)dpdata/system.py (8)
MultiSystems(1360-1729)System(55-1111)register_data_type(1094-1111)LabeledSystem(1159-1357)copy(429-431)get_nframes(417-419)get_nframes(1495-1497)formula(611-620)tests/comp_sys.py (2)
MultiSystems(174-183)MSAllIsNoPBC(207-213)
🪛 Pylint (3.3.7)
tests/test_deepmd_mixed.py
[error] 18-21: Unable to import 'dpdata.data_type'
(E0401)
[convention] 463-463: Missing class docstring
(C0115)
[refactor] 463-463: Too many instance attributes (12/7)
(R0902)
[refactor] 466-466: Too many local variables (19/15)
(R0914)
[convention] 557-557: Missing function or method docstring
(C0116)
[convention] 561-561: Missing function or method docstring
(C0116)
[convention] 565-565: Missing function or method docstring
(C0116)
[convention] 571-571: Missing function or method docstring
(C0116)
[convention] 586-586: Missing function or method docstring
(C0116)
🔇 Additional comments (4)
tests/test_deepmd_mixed.py (4)
18-21: Import statements look good.The imports for
AxisandDataTypeare appropriate for defining new data types in the test.🧰 Tools
🪛 Pylint (3.3.7)
[error] 18-21: Unable to import 'dpdata.data_type'
(E0401)
472-489: Well-structured data type registration.The definition and registration of new optional data types follows the proper pattern using
DataTypewith appropriate shapes and therequired=Falseflag.
571-584: Comprehensive fparam validation.The test properly verifies both the existence of
fparamfields and their numerical integrity after the save/load cycle.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 571-571: Missing function or method docstring
(C0116)
586-599: Comprehensive aparam validation.The test properly verifies both the existence of
aparamfields and their numerical integrity after the save/load cycle, following the same pattern as the fparam test.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 586-586: Missing function or method docstring
(C0116)
There was a problem hiding this comment.
Please check other files in this directory to see how we support ANY new keys with the plugin system.
Lines 49 to 56 in 08aefba
There was a problem hiding this comment.
Please check other files in this directory to see how we support ANY new keys with the plugin system.
@njzjz I am not quite following. Are you suggesting that there are missing changes need to be added, or you want the feature to be implemented using a different approach. This PR only fix the fparam bug in mixed systems.
There was a problem hiding this comment.
fparam is implemented with plugins, so I don't suggest handling it specially. Ideally, we should handle any registered data type.
There was a problem hiding this comment.
fparam is implemented with plugins, so I don't suggest handling it specially. Ideally, we should handle any registered data type.
If I understand correctly, you are suggesting refactoring the temp_idx logic into comp.py, not only for fparam but also include all the other dtypes?
Lines 44 to 69 in 08aefba
That probably be done in a separate PR as a refactor. This PR only aims to fix the bug.
3 participants
This PR should fix #831
Summary by CodeRabbit
New Features
Tests