Use dataclass interface to spglib symmetry #248
Conversation
spglib.get_symmetry_dataset now returns a dataclass and throws warnings on __getitem__. This affects our Symmetry class when using the .info attribute.
WalkthroughThe modification in the Changes
Poem
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 as PR comments)
Additionally, you can add 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- structuretoolkit/analyse/symmetry.py (2 hunks)
Additional comments not posted (1)
structuretoolkit/analyse/symmetry.py (1)
345-350: LGTM! Ensure downstream compatibility.The use of
dataclasses.asdictto convert the output ofspglib.get_symmetry_datasetinto a dictionary format is correct and enhances usability. However, ensure that any downstream logic that relies on the structure of the returned data is updated to accommodate this change.The code changes are approved.
Run the following script to verify the impact on downstream logic:
Verification successful
Downstream Logic Verified: No Issues Found
The change to use
dataclasses.asdictin theinfoproperty method is compatible with the existing codebase. The tests intests/test_symmetry.pyandtests/test_analyse_symmetry.pyconfirm that the dictionary format is expected and should work as intended. Ensure that the dictionary keys remain consistent with previous versions to avoid any issues.
- No changes needed in the downstream logic as per the current test coverage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the updated `info` property method on downstream logic. # Test: Search for the usage of the `info` property. Expect: Ensure compatibility with the new dictionary format. rg --type python -A 5 $'info'Length of output: 59
Script:
#!/bin/bash # Corrected: Verify the impact of the updated `info` property method on downstream logic. # Test: Search for the usage of the `info` property. Expect: Ensure compatibility with the new dictionary format. rg --type py -A 5 $'info'Length of output: 21236
|
@pmrv As far as I understand dataclasses were only recently introduced in |
|
Right, I hadn't considered. I'll put a patch tomorrow so that old versions work too. |
Note to myself: This is wrong, the commit actually introduces a new bug when the returned info is |
spglib<2.5 returns a dict, not a dataclass, so apply asdict only when necessary.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- structuretoolkit/analyse/symmetry.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- structuretoolkit/analyse/symmetry.py
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- structuretoolkit/analyse/symmetry.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- structuretoolkit/analyse/symmetry.py
spglib.get_symmetry_dataset now returns a dataclass and throws warnings on getitem. This affects our Symmetry class when using the .info attribute.
Summary by CodeRabbit
New Features
Bug Fixes
None, ensuring stability.