Add support for converting move flags between Abacus and VASP #744
Conversation
CodSpeed Performance ReportMerging #744 will not alter performanceComparing Summary
|
📝 WalkthroughWalkthroughThe pull request introduces modifications across several files in 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 (
|
|
Currently, only input files POSCAR for VASP and STRU for Abacus are supported. The move flags in their output files have not been handled yet. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
tests/test_vasp_poscar_dump.py (1)
37-50: Add cleanup and enhance test coverage.While the test verifies basic move flags functionality, there are several improvements that could make it more robust:
- The test creates temporary files but doesn't clean them up
- Only tests one specific case with hardcoded coordinates
- No error cases are verified
Consider applying these improvements:
def test_dump_move_flags(self): tmp_system = dpdata.System() tmp_system.from_vasp_poscar(os.path.join("poscars", "POSCAR.oh.c")) tmp_system.to_vasp_poscar("tmp.POSCAR") self.system = dpdata.System() self.system.from_vasp_poscar("tmp.POSCAR") with open("tmp.POSCAR") as f: content = f.read() + try: + stru_ref = """Cartesian + 0.0000000000 0.0000000000 0.0000000000 T T F + 1.2621856044 0.7018027835 0.5513883414 F F F +""" + self.assertTrue(stru_ref in content) + finally: + if os.path.exists("tmp.POSCAR"): + os.remove("tmp.POSCAR") +def test_dump_move_flags_error(self): + """Test error handling for invalid move flags""" + with self.assertRaises(ValueError): + tmp_system = dpdata.System() + # Test with invalid move flags + tmp_system.coords = [[0, 0, 0], [1, 1, 1]] + tmp_system.move_flags = [[True, True]] # Invalid shape + tmp_system.to_vasp_poscar("tmp.POSCAR")tests/test_abacus_stru_dump.py (1)
106-139: LGTM with suggestions for improvementThe test effectively verifies the conversion of move flags between VASP POSCAR and ABACUS STRU formats, covering both default and custom move flag scenarios. A few suggestions to enhance the test:
- Add a docstring explaining the test's purpose and expected behavior
- Consider adding error case tests (e.g., invalid move flag arrays)
- Add assertions to verify file cleanup in tearDown
Example docstring:
def test_dump_move_from_vasp(self): """Test conversion of move flags from VASP POSCAR to ABACUS STRU format. Verifies: 1. Default move flags are correctly converted 2. Custom move flags are properly translated to ABACUS format (1/0) """dpdata/abacus/md.py (1)
224-225: Consider adding type checking for move dataWhile the length check is good, consider making the condition more robust by explicitly checking the type and shape of the move array.
- if len(move) > 0: + if isinstance(move, np.ndarray) and move.size > 0: data["move"] = movedpdata/abacus/scf.py (3)
313-313: Add defensive handling for move array conversionThe conversion to boolean array should handle cases where
movemight be empty or None to prevent potential runtime errors.- move = np.array(move, dtype=bool) + move = np.array(move, dtype=bool) if move else np.array([], dtype=bool)
514-515: Enhance move data validationThe condition for adding move data could be more explicit about handling None values.
- if len(move) > 0: + if move is not None and len(move) > 0: data["move"] = move
673-675: Consider a more concise implementationThe move flag extraction could be written more concisely using dict.get() with a default value.
- if move is None and data.get("move", None) is not None and len(data["move"]) > 0: - move = data["move"] + move = move or (data.get("move") if data.get("move") and len(data["move"]) > 0 else None)dpdata/vasp/poscar.py (1)
61-62: Simplify nestedifstatementsAs suggested by the static analysis tool (SIM102), you can combine the nested
ifstatements into a single condition usingand.Apply this diff to simplify the
ifstatements:- if not is_cartesian: - if lines[7][0] not in ["d", "D"]: + if not is_cartesian and lines[7][0] not in ["d", "D"]: raise RuntimeError( "seem not to be a valid POSCAR of vasp 5.x, may be a POSCAR of vasp 4.x?" )🧰 Tools
🪛 Ruff
61-62: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- dpdata/abacus/md.py (2 hunks)
- dpdata/abacus/relax.py (2 hunks)
- dpdata/abacus/scf.py (6 hunks)
- dpdata/vasp/poscar.py (5 hunks)
- tests/abacus.scf/STRU.ch4 (1 hunks)
- tests/abacus.scf/stru_test (1 hunks)
- tests/test_abacus_stru_dump.py (1 hunks)
- tests/test_vasp_poscar_dump.py (1 hunks)
- tests/test_vasp_poscar_to_system.py (1 hunks)
🧰 Additional context used
🪛 Ruff
dpdata/vasp/poscar.py
61-62: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
🔇 Additional comments (15)
tests/abacus.scf/stru_test (2)
29-32: Verify move flag compatibility between ABACUS and VASP.The move flags in this test file (
0/1format) need to match VASP's selective dynamics format (T/F). Please ensure that:
- These move flag combinations are valid in both ABACUS and VASP
- The conversion between formats preserves the constraints correctly
Let's verify the conversion implementation:
#!/bin/bash # Search for move flag conversion logic rg -A 5 "def.*move"
29-32: Consider expanding test coverage for move flags.The modified move flags for hydrogen atoms provide a good starting point for testing atomic constraints:
0 0 0: Completely fixed atom1 0 1: Atom fixed in y-direction0 1 1: Atom fixed in x-directionHowever, consider adding test cases for:
1 1 1: Completely free atom (already covered by carbon)1 1 0: Atom fixed in z-direction0 1 0: Atom movable only in y-directionLet's verify if these test cases are covered elsewhere:
tests/abacus.scf/STRU.ch4 (3)
26-28: Review the hydrogen atoms' asymmetric move flag configuration.The hydrogen atoms have different movement constraints:
- H1 & H2: Movable in x,z (1 0 1)
- H3: Movable in y,z (0 1 1)
This asymmetric configuration might be intentional for testing different movement scenarios, but it's worth verifying if this represents a valid test case for CH4 geometry optimization.
Consider adding a comment explaining the rationale behind these specific movement constraints to help other developers understand the test case design.
21-21: Verify chemical validity of the CH4 structure.The atomic positions and constraints should maintain a tetrahedral geometry typical of CH4:
- One central carbon atom
- Four hydrogen atoms
- C-H bond lengths should be approximately equal
- H-C-H angles should be approximately tetrahedral (109.5°)
#!/bin/bash # Description: Check for documentation or comments about expected molecular geometry # Expect: Information about tetrahedral structure validation # Search for related test documentation rg -l "CH4|methane|tetrahedral" tests/ | grep -i "readme\|test.*\.md"Also applies to: 26-28
21-21: Verify the carbon atom's move flags configuration.The carbon atom is set to be movable in all directions (1 1 1). Given that carbon is the central atom in a CH4 molecule, this configuration allows for proper geometry optimization.
✅ Verification successful
Move flags configuration is consistent with molecular structure requirements
The carbon atom's move flags (1 1 1) in STRU.ch4 follow a consistent pattern seen across the test files. Looking at the evidence:
- In STRU.ch4, the central carbon atom's configuration allows movement in all directions (1 1 1), which is appropriate for geometry optimization of a methane molecule.
- The hydrogen atoms have varying move flags that create a balanced constraint system:
- Two H atoms with (1 0 1)
- One H atom with (0 1 1)
- One H atom with (0 0 0)
- Other test files (stru.ref, stru_test) show similar patterns where central atoms often use (1 1 1) flags while peripheral atoms have varied constraints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if other test files maintain similar move flag patterns for central atoms # Expect: Similar move flag patterns (1 1 1) for central atoms in molecular structures # Search for similar atomic position patterns in other test files rg -A 1 "^C\s+#" tests/Length of output: 163
Script:
#!/bin/bash # Let's try a different approach to find atomic structure files and analyze their patterns # First, let's look at the content of the STRU.ch4 file to understand its format better cat tests/abacus.scf/STRU.ch4 # Then search for similar STRU files fd STRU tests/ # Also search for any files containing atomic coordinates with move flags rg "^\s*[0-9]+\.[0-9]+\s+[0-9]+\.[0-9]+\s+[0-9]+\.[0-9]+\s+[0-1]\s+[0-1]\s+[0-1]" tests/Length of output: 7763
tests/test_vasp_poscar_to_system.py (1)
17-19: Verify test data and enhance test documentation.The test verifies move flags for atomic coordinates, but could benefit from additional documentation and validation.
Let's verify the test data exists and contains the expected move flags:
Consider adding:
- A docstring explaining the test's purpose and the expected move flags format
- Comments explaining what each atom's move flags represent
def test_move_flags(self): + """Verify that selective dynamics flags are correctly read from POSCAR. + + Expected format: + - First atom: [T T F] (movable in x,y, fixed in z) + - Second atom: [F F F] (fixed in all directions) + """ expected = np.array([[True, True, False], [False, False, False]]) self.assertTrue(np.array_equal(self.system["move"], expected))✅ Verification successful
Let me try to locate the test file using a more thorough search approach.
I found that
tests/poscars/POSCAR.oh.cexists. Let's examine its content to verify the move flags.
Test data matches expectations, but documentation can be improved
The test data in
tests/poscars/POSCAR.oh.cperfectly matches the expected move flags in the test:
- First atom (0.00 0.00 0.00): T T F matches [True, True, False]
- Second atom (0.25 0.25 0.25): F F F matches [False, False, False]
Consider adding a docstring to improve test clarity:
def test_move_flags(self): """Verify selective dynamics flags are correctly parsed from POSCAR. The test file (POSCAR.oh.c) contains two atoms with selective dynamics: - First atom (O): movable in x,y planes but fixed in z direction - Second atom (H): fixed in all directions """ expected = np.array([[True, True, False], [False, False, False]]) self.assertTrue(np.array_equal(self.system["move"], expected))🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify POSCAR test file exists and check its content # Expected: File exists and contains selective dynamics flags matching [[T T F], [F F F]] # Test 1: Check if test file exists if [ -f "poscars/POSCAR.oh.c" ]; then echo "Test file exists" # Test 2: Extract and display selective dynamics flags cat poscars/POSCAR.oh.c else echo "Test file not found" fiLength of output: 81
Script:
#!/bin/bash # Search for POSCAR files in the repository fd POSCAR # Also check if there are any test files related to POSCAR fd -t f -e py test.*poscarLength of output: 515
Script:
#!/bin/bash # Display content of the POSCAR file cat tests/poscars/POSCAR.oh.c # Also let's see the full test file for context cat tests/test_vasp_poscar_to_system.pyLength of output: 2355
tests/test_vasp_poscar_dump.py (1)
37-37: Expand test coverage for Abacus-VASP conversion flow.The current test only verifies VASP POSCAR output. To fully validate the PR objective of converting move flags between Abacus and VASP, consider adding tests for:
- Converting from Abacus to VASP format with move flags
- Converting from VASP to Abacus format with move flags
- Round-trip conversion (Abacus → VASP → Abacus)
Let's verify if there are corresponding test files for Abacus format:
dpdata/abacus/relax.py (2)
186-186: Update documentation for the modified get_coords function.The function signature has been updated to include the
moveparameter, but the documentation should be updated to reflect this change.Let's verify the get_coords implementation:
221-222: LGTM! Verify test coverage for move data handling.The conditional inclusion of move data follows the established pattern for optional data in the output dictionary.
Let's verify the test coverage:
✅ Verification successful
Move data handling is properly tested and consistent
The move data handling in
dpdata/abacus/relax.pyis well-tested through:
- Unit tests in
test_abacus_stru_dump.pythat verify move flags functionality- Consistent implementation across abacus modules (scf.py, relax.py, md.py) following the same pattern
- Proper parsing and validation in the STRU format parser (
parse_stru_pos)The conditional inclusion of move data follows the established pattern used for other optional data like magmom and magforce, and is correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests related to move data handling rg -l "move.*data\[.move.\]|data\[.move.\].*move" --type py tests/Length of output: 68
Script:
#!/bin/bash # Let's try a broader search for move-related tests rg -l "move" tests/ --type py # Also search for test files specifically related to abacus relax fd "test.*abacus.*relax.*\.py$" tests/ # Look for actual test implementations mentioning move rg "def test.*move" tests/ --type py # Check the test file content if it exists fd "test_abacus_relax.py" tests/ -x cat {}Length of output: 7960
Script:
#!/bin/bash # Let's examine the implementation of move data handling in the abacus relax module rg -A 5 -B 5 "move.*=.*move" dpdata/abacus/relax.py # Also check if there are any other test files that might contain move-related tests fd "test.*abacus.*\.py$" tests/ -x cat {} | rg -A 5 -B 5 "move" # Look for any move-related functionality in the abacus module rg "move" dpdata/abacus/ --type pyLength of output: 10118
tests/test_abacus_stru_dump.py (2)
101-104: LGTM: Test data correctly updated with move flagsThe test data has been appropriately updated to include move flags (0 0 0) for the atom positions, maintaining consistency with the new move flag support.
106-139: Verify scope of move flag conversion implementationThe current implementation successfully adds support for move flag conversion between VASP POSCAR and ABACUS STRU input files. However, as noted in the PR description, support for move flags in output files is still pending.
Would you like me to help create a GitHub issue to track the remaining work for output files support?
dpdata/abacus/md.py (2)
170-172: LGTM: Move flag support added correctlyThe update to unpack the
moveparameter fromget_coordsis implemented correctly and aligns with the PR objective of supporting move flags between Abacus and VASP.
Line range hint
170-225: Verify test coverage for move flagsSince this is a new feature for converting move flags between Abacus and VASP, let's verify that appropriate test cases exist.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about the test coverage for move flags.
Move flags functionality is well-tested across multiple test files
The codebase has comprehensive test coverage for move flags:
test_vasp_poscar_dump.py: Tests dumping of move flagstest_abacus_stru_dump.py: Tests conversion of move flags from VASP to ABACUS formattest_vasp_poscar_to_system.py: Tests correct parsing of move flags with expected values🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test coverage of move flags functionality # Test 1: Look for test files related to move flags echo "Searching for test files related to move flags..." rg -l "move.*flag|test.*move" "tests/" # Test 2: Check for test cases specifically testing the move parameter echo "Checking for test cases of move parameter..." rg -A 5 "def test.*move" "tests/"Length of output: 2150
dpdata/abacus/scf.py (2)
481-481: LGTM!The unpacking of the move flag from get_coords() is implemented correctly.
Line range hint
1-858: Verify integration with VASP move flagsWhile the ABACUS move flag implementation looks good, we should verify that it integrates correctly with the VASP side of the conversion.
| stru_ref = """Cartesian | ||
| 0.0000000000 0.0000000000 0.0000000000 T T F | ||
| 1.2621856044 0.7018027835 0.5513883414 F F F | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameterizing the test data.
The hardcoded coordinates and move flags in the reference string make the test brittle and harder to maintain.
Consider moving the test data to a separate fixture file or using parameterized tests:
# In a separate test_data.py file
MOVE_FLAGS_TEST_CASES = [
{
'coords': [[0.0, 0.0, 0.0], [1.2621856044, 0.7018027835, 0.5513883414]],
'move_flags': [[True, True, False], [False, False, False]],
'expected_output': """Cartesian
0.0000000000 0.0000000000 0.0000000000 T T F
1.2621856044 0.7018027835 0.5513883414 F F F
"""
},
# Add more test cases here
]Then use @pytest.mark.parametrize or similar to run multiple test cases.
Angel-Jia
force-pushed
the
yaosk/atoms_flag
branch
from
9eb7227
to
ed6aa65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
dpdata/vasp/poscar.py (2)
56-66: Simplify conditions and improve error messageThe implementation can be improved for better readability and user experience.
Apply these changes:
- if lines[7][0] not in ["d", "D"]: - raise RuntimeError( - "seem not to be a valid POSCAR of vasp 5.x, may be a POSCAR of vasp 4.x?" - ) + if not is_cartesian and lines[7][0] not in ["d", "D"]: + raise RuntimeError( + "Invalid POSCAR format: Expected 'Direct' or 'Cartesian' coordinates. " + "This might be a VASP 4.x POSCAR format which is not supported." + )🧰 Tools
🪛 Ruff
61-62: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
Line range hint
91-126: Add type hints and docstring for better maintainabilityThe move flags handling logic is correct but would benefit from better documentation.
Consider adding:
- Type hints for the move flags:
def from_system_data( system: dict[str, np.ndarray], f_idx: int = 0, skip_zeros: bool = True ) -> str:
- Docstring explaining the move flags format:
"""Convert system data to VASP POSCAR format. Args: system: Dictionary containing system data with optional 'move' key for selective dynamics flags. f_idx: Frame index for systems with multiple frames. skip_zeros: Whether to skip atoms with zero count. Returns: POSCAR format string. The 'move' key in system can contain either: - A list of 3 booleans per atom for x,y,z directions - A single boolean per atom applied to all directions """tests/test_abacus_stru_dump.py (1)
149-190: Add documentation and enhance test coverage.While the test implementation correctly validates the move flags conversion between VASP and ABACUS formats, consider the following improvements:
- Add a docstring explaining the test's purpose and scenarios
- Add assertions to verify the complete file content, not just the reference structure
- Add test cases for invalid move flag scenarios
Example docstring and additional test cases:
def test_dump_move_from_vasp(self): """Test conversion of move flags from VASP POSCAR to ABACUS STRU format. Scenarios: 1. Default move flags from POSCAR 2. Custom move flags override """ # ... existing code ... # Additional test case for invalid move flags with self.assertRaises(ValueError): self.system.to( "abacus/stru", "STRU_tmp", pp_file={"O": "O.upf", "H": "H.upf"}, move=[[True, False]], # Invalid length )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- dpdata/abacus/md.py (2 hunks)
- dpdata/abacus/relax.py (2 hunks)
- dpdata/abacus/scf.py (6 hunks)
- dpdata/vasp/poscar.py (5 hunks)
- tests/abacus.scf/STRU.ch4 (1 hunks)
- tests/abacus.scf/stru_test (1 hunks)
- tests/test_abacus_stru_dump.py (1 hunks)
- tests/test_vasp_poscar_dump.py (1 hunks)
- tests/test_vasp_poscar_to_system.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- dpdata/abacus/md.py
- dpdata/abacus/relax.py
- tests/abacus.scf/STRU.ch4
- tests/abacus.scf/stru_test
- tests/test_vasp_poscar_dump.py
- tests/test_vasp_poscar_to_system.py
🧰 Additional context used
🪛 Ruff
dpdata/vasp/poscar.py
61-62: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
🔇 Additional comments (9)
dpdata/vasp/poscar.py (3)
8-14: Make flag comparison case-insensitiveThe current implementation is case-sensitive, which could cause issues with lowercase input.
7-7: LGTM! Well-structured changes for move flags supportThe implementation correctly:
- Maintains backward compatibility with selective_dynamics parameter
- Guards move flags collection appropriately
- Ensures consistent data types with numpy array conversion
Also applies to: 22-22, 38-39, 50-50
Line range hint
7-126: Verify consistent move flags handling between Abacus and VASPLet's ensure the move flags conversion is consistent between both formats.
✅ Verification successful
Based on the search results, I can now make a determination about the move flags handling between VASP and Abacus formats. The search revealed:
VASP's implementation in
dpdata/vasp/poscar.pyhandles move flags with:
- Clear conversion between T/F text format and boolean arrays
- Support for both reading and writing selective dynamics
- Proper validation of flag values
Abacus's implementation in
dpdata/abacus/scf.pyshows:
- Basic move flags structure exists
- Documentation indicates similar xyz direction flags per atom
- No inconsistencies in the implementation approach
Let me verify the actual conversion implementation:
Based on the comprehensive analysis of the code, I can now provide a definitive response about the move flags handling between VASP and Abacus formats:
Move flags implementation is consistent between formats
VASP format:
- Reads T/F flags from POSCAR and converts to boolean array
- Stores in system["move"] as numpy boolean array
- Writes back as T/F in POSCAR format
Abacus format:
- Reads 0/1 flags from STRU file and converts to boolean array
- Stores in system["move"] as numpy boolean array
- Writes back as 0/1 in STRU format
The implementation ensures consistent handling because:
- Both formats store move flags in the same system["move"] boolean array format internally
- The conversion functions properly map between their respective text formats (T/F ↔ bool ↔ 0/1)
- Test coverage exists for VASP move flags handling
- The Abacus implementation follows the same pattern with appropriate format-specific conversions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent move flags handling between Abacus and VASP # Test 1: Find all files handling move flags echo "Files handling move flags:" rg -l "move.*flags?" --type python # Test 2: Check for potential inconsistencies in move flags handling echo -e "\nPotential inconsistencies in move flags handling:" rg -A 3 "def.*move" --type pythonLength of output: 312
Script:
#!/bin/bash # Let's try without file type filtering since python extension wasn't recognized # Search for move flags handling echo "Files handling move flags:" rg -l "move.*flags?" # Check implementations related to move flags echo -e "\nMove flags implementations:" rg -A 5 "move.*flags?" # Search for Abacus and VASP format conversions echo -e "\nFormat conversion implementations:" rg -A 10 "to_system_data|from_system_data"Length of output: 31786
Script:
#!/bin/bash # Check the actual conversion implementation between formats echo "Abacus move flags conversion implementation:" rg -A 10 "def.*system.*data" dpdata/abacus/scf.py echo -e "\nTests covering move flags conversion:" rg -l "test.*move.*flags?" tests/Length of output: 344
Script:
#!/bin/bash # Let's check the test implementations and actual conversion code echo "VASP move flags test implementation:" rg -A 10 "test_move_flags" tests/test_vasp_poscar_to_system.py echo -e "\nVASP dump move flags test:" rg -A 10 "test_dump_move_flags" tests/test_vasp_poscar_dump.py echo -e "\nAbacus move flags handling in scf.py:" cat dpdata/abacus/scf.pyLength of output: 32840
tests/test_abacus_stru_dump.py (1)
144-147: LGTM: Move flags correctly added to test case.The test case has been appropriately updated to include move flags (0 0 0) in the reference structure, which aligns with the PR's objective of supporting move flags between ABACUS and VASP.
dpdata/abacus/scf.py (5)
481-481: Updateget_frameto handlemovedata appropriatelyThe
get_framefunction now unpacksmovefromget_coords. Ensure that themovedata is correctly utilized within the function and integrated into thedatadictionary.
514-515: Addingmovedata to thedatadictionaryThe
movedata is conditionally added to thedatadictionary when it exists. This change ensures that movement flags are included in the output data.
567-567: Updateget_frame_from_struto handlemovedataThe
get_frame_from_strufunction now unpacksmovefromget_coords. Verify thatmoveis correctly handled and integrated into thedatadictionary.
577-578: Includingmovedata in thedatadictionaryThe
movedata is conditionally added to thedatadictionary if it exists, ensuring movement flags are captured. This enhancement improves the functionality of the function.
313-314: Ensure all callers handle the additional 'move' return value fromget_coordsThe function
get_coordsnow returnsmoveas an additional output. Please verify that all calls toget_coordsin the codebase have been updated to handle this new return value to prevent potential issues.✅ Verification successful
Let me analyze the findings:
There are two different
get_coordsfunctions:
- One in
dpdata/qe/scf.pythat returns 4 values- One in
dpdata/abacus/scf.pythat returns 5 values (includingmove)All calls to the ABACUS version (in
dpdata/abacus/) correctly unpack 5 values:
dpdata/abacus/md.py:atom_names, natoms, types, coords, move = get_coords(...)dpdata/abacus/relax.py:atom_names, natoms, types, coord_tmp, move = get_coords(...)- Two calls in
dpdata/abacus/scf.py:atom_names, natoms, types, coords, move = get_coords(...)The call in
dpdata/qe/scf.pyis to its own localget_coordsfunction, not the ABACUS one.Let me generate the final response:
All callers correctly handle the 'move' return value from ABACUS
get_coordsThe codebase analysis shows that all calls to the ABACUS version of
get_coordsindpdata/abacus/scf.pycorrectly unpack all five return values, including themoveparameter. The only otherget_coordscall found is in the QE module, but it refers to a different function with a different signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `get_coords` and check if they unpack the `move` return value. # Expected result: All calls should unpack 5 return values from `get_coords`. rg --type py 'get_coords\(' --context 1Length of output: 1399
dpdata/abacus/scf.py
Outdated
| if move is None and data.get("move", None) is not None and len(data["move"]) > 0: | ||
| move = data["move"] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify condition when assigning move in make_unlabeled_stru
Consider simplifying the condition for assigning move from data. Checking len(data["move"]) > 0 may not be necessary if you want to assign move whenever it exists in data.
Apply this diff to simplify the condition:
-if move is None and data.get("move", None) is not None and len(data["move"]) > 0:
+if move is None and data.get("move", None) is not None:
move = data["move"]📝 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 move is None and data.get("move", None) is not None and len(data["move"]) > 0: | |
| move = data["move"] | |
| if move is None and data.get("move", None) is not None: | |
| move = data["move"] | |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #744 +/- ##
==========================================
+ Coverage 84.92% 85.07% +0.14%
==========================================
Files 81 81
Lines 7418 7478 +60
==========================================
+ Hits 6300 6362 +62
+ Misses 1118 1116 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
The newly defined data type "move" should be registered. for example
dpdata/dpdata/plugins/abacus.py
Line 49 in 7853b7b
otherwise, it will not handled by dpdata, which may be tested by load and dump the same poscar file.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
dpdata/plugins/abacus.py (2)
68-76: LGTM! The move flags implementation is correct.The registration of the "move" data type with
dpdata.Systemis appropriate as movement constraints are part of the basic structure data, not labels. The shape(NFRAMES, NATOMS, 3)correctly represents 3D movement constraints per atom per frame.Consider renaming the
register_mag_datafunction to better reflect its expanded responsibility, e.g.,register_atomic_dataorregister_structure_data, as it now handles both magnetic and movement data.
68-76: Add documentation for the move flags data type.Please add docstring documentation explaining:
- The purpose and structure of the "move" data type
- The meaning of the 3D values (e.g., True/False for each direction?)
- The relationship between VASP and ABACUS move flags
Example:
def register_atomic_data(data): """Register atomic-level data types for the system. Parameters ---------- data : dict System data dictionary that may contain: - "spins": Magnetic spin vectors (3D) for each atom - "mag_forces": Magnetic forces (3D) on each atom - "move": Movement constraints (3D) for each atom where True/False indicates if movement is allowed in each direction (x,y,z) """dpdata/vasp/poscar.py (1)
59-69: Simplify nested if statementsThe nested if statements can be combined for better readability.
Apply this diff:
- if not is_cartesian: - if lines[7][0] not in ["d", "D"]: + if not is_cartesian and lines[7][0] not in ["d", "D"]: raise RuntimeError( "seem not to be a valid POSCAR of vasp 5.x, may be a POSCAR of vasp 4.x?" )🧰 Tools
🪛 Ruff
64-65: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
dpdata/plugins/vasp.py (1)
18-27: LGTM! Consider adding a docstring.The implementation correctly registers the move data type with appropriate dimensions. The conditional registration and optional flag are good practices.
Add a docstring to improve code documentation:
def register_move_data(data): + """Register move data type if present in the data dictionary. + + Parameters + ---------- + data : dict + Dictionary containing system data + """ if "move" in data: dt = DataType(dpdata/abacus/scf.py (2)
302-303: Add input validation for move flags.The move flags should be validated to ensure they only contain valid values (0 or 1) before converting to boolean type. This would prevent potential issues with invalid input data.
Consider adding validation before the conversion:
if imove is not None: + if not all(x in [0, 1] for x in imove): + raise ValueError(f"Invalid move flag values. Expected 0 or 1, got {imove}") move.append(imove) move = np.array(move, dtype=bool)Also applies to: 314-315
482-482: LGTM! Consider adding docstring for move flag.The implementation correctly handles the move flags. Consider adding documentation about the move flag format in the function's docstring.
Add to docstring:
Returns ------- ... move : numpy.ndarray Boolean array indicating which coordinates can move during geometry optimizationAlso applies to: 515-516
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- dpdata/abacus/scf.py (7 hunks)
- dpdata/plugins/abacus.py (1 hunks)
- dpdata/plugins/vasp.py (4 hunks)
- dpdata/vasp/poscar.py (5 hunks)
- tests/test_vasp_poscar_to_system.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_vasp_poscar_to_system.py
🧰 Additional context used
🪛 GitHub Check: codecov/patch
dpdata/vasp/poscar.py
[warning] 14-14: dpdata/vasp/poscar.py#L14
Added line #L14 was not covered by tests
[warning] 121-122: dpdata/vasp/poscar.py#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 124-124: dpdata/vasp/poscar.py#L124
Added line #L124 was not covered by tests
🪛 Ruff
dpdata/vasp/poscar.py
64-65: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
🔇 Additional comments (7)
dpdata/plugins/abacus.py (1)
68-76: Verify move flags conversion implementation.While the data type registration is in place, we should verify that the actual conversion logic between ABACUS and VASP move flags is implemented in the respective format handlers.
✅ Verification successful
Based on the search results, I can now generate the final response:
Move flags conversion is properly implemented and verified.
The codebase shows:
- Move flags are correctly registered in both ABACUS and VASP plugins
- VASP has a dedicated
register_move_datafunction that handles move flags- Test coverage exists for move flags conversion:
test_vasp_poscar_to_system.pyverifies move flags are correctly read from POSCARtest_vasp_poscar_dump.pyverifies move flags are correctly written to POSCAR🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for move flags handling in ABACUS and VASP format handlers echo "Checking for move flags handling in get_frame functions..." rg -l "def get_frame.*move" echo "\nChecking for move flags conversion logic..." rg -l "move.*vasp.*abacus|abacus.*move.*vasp" echo "\nChecking for test coverage of move flags conversion..." rg -l "test.*move.*vasp.*abacus|test.*abacus.*move.*vasp"Length of output: 488
Script:
#!/bin/bash # Let's try a more targeted search approach echo "Checking ABACUS format handler for move flags..." rg -A 5 -B 5 "move" dpdata/plugins/abacus.py echo "\nChecking VASP format handler for move flags..." rg -A 5 -B 5 "move" dpdata/plugins/vasp.py echo "\nChecking test files for move flags..." fd "test.*\.py" | xargs rg -l "move" echo "\nChecking get_frame implementations..." ast-grep --pattern 'def get_frame($$$) { $$$ }'Length of output: 3078
Script:
#!/bin/bash # Let's examine the test files for ABACUS and VASP move flags specifically echo "Checking ABACUS test files for move flags implementation..." rg -A 5 -B 5 "move" tests/test_abacus_md.py tests/test_abacus_relax.py tests/test_abacus_pw_scf.py echo "\nChecking VASP POSCAR test files for move flags implementation..." rg -A 5 -B 5 "move" tests/test_vasp_poscar_to_system.py tests/test_vasp_poscar_dump.py echo "\nChecking data conversion functions in ABACUS plugin..." rg -A 10 "def to_system_data" dpdata/plugins/abacus.pyLength of output: 10125
dpdata/vasp/poscar.py (2)
8-14: Make move flag handling more robustThe past review comment about case-insensitive flag handling is still valid. Additionally, the error handling code path is not covered by tests.
Let's verify the test coverage:
#!/bin/bash # Description: Check test coverage for move flag error handling rg -l "Invalid selective dynamics flag" tests/🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 14-14: dpdata/vasp/poscar.py#L14
Added line #L14 was not covered by tests
38-39: LGTM: Move flags collection is well-implementedThe code correctly collects move flags when selective dynamics is enabled and properly reshapes them into the required format.
Also applies to: 50-53
dpdata/plugins/vasp.py (3)
41-41: LGTM! Appropriate placement of move data registration.The registration is correctly placed after data processing and atom name uniquification.
Line range hint
18-153: Verify test coverage for move flag handling.While the implementation is consistent across formats, we should ensure adequate test coverage for move flag handling in all VASP formats (POSCAR, OUTCAR, and XML).
#!/bin/bash # Description: Check test coverage for move flag handling # Test: Search for move-related tests rg -l "test.*move" "tests/"
116-116: Verify move flag support in OUTCAR format.While the registration is placed correctly, we should verify that the OUTCAR format actually supports move flags and that they are being properly extracted in
dpdata.vasp.outcar.get_frames.dpdata/abacus/scf.py (1)
568-568: LGTM! Implementation matches get_frame.The move flag handling is consistent with the implementation in get_frame.
Also applies to: 578-579
| if isinstance(move, np.ndarray): | ||
| move = move.tolist() | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for invalid move flags error handling
The review comment is correct. Based on the test files inspection:
- There are only basic happy path tests for move flags in
test_move_flags()andtest_dump_move_flags() - No tests cover the error handling case when invalid move flags are provided
- The error path
raise RuntimeError("Invalid move flags...")is untested
Add test cases to cover:
- Invalid move flags type (e.g., string, dict)
- List with incorrect length (not 3)
- None value
🔗 Analysis chain
Improve test coverage and simplify move flags formatting
- The error handling for invalid move flags (lines 124-126) is not covered by tests.
- The type checking and formatting logic could be simplified.
Let's verify the test coverage for invalid move flags:
Consider simplifying the move flags formatting:
- if isinstance(move_flags, list) and len(move_flags) == 3:
- line += " " + " ".join(["T" if flag else "F" for flag in move_flags])
- elif isinstance(move_flags, (int, float, bool)):
- line += " " + " ".join(["T" if move_flags else "F"] * 3)
- else:
- raise RuntimeError(
- f"Invalid move flags: {move_flags}, should be a list or a bool"
- )
+ flags = move_flags if isinstance(move_flags, list) else [bool(move_flags)] * 3
+ if not isinstance(flags, list) or len(flags) != 3:
+ raise RuntimeError(f"Invalid move flags: {move_flags}, should be a list or a bool")
+ line += " " + " ".join("T" if flag else "F" for flag in flags)Also applies to: 117-126
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for invalid move flags error handling
rg -l "Invalid move flags.*should be a list or a bool" tests/
Length of output: 63
Script:
#!/bin/bash
# Let's check for test files related to POSCAR and move flags
fd "test.*poscar.*\.py" tests/
# Then look for any test cases involving move flags
rg -l "move.*flags?" tests/
Length of output: 196
Script:
#!/bin/bash
# Let's also check the actual implementation and error handling
rg -A 3 "Invalid move flags" dpdata/
Length of output: 260
Script:
#!/bin/bash
# Check test files content for move flags testing
ast-grep --pattern 'def test_$_($$$) {
$$$
move_$$$
$$$
}'
Length of output: 66
Script:
#!/bin/bash
# Let's check the content of the test files for move flags testing
rg -B2 -A5 "move.*flags?" tests/test_vasp_poscar_dump.py tests/test_vasp_poscar_to_system.py
Length of output: 1286
Script:
#!/bin/bash
# Let's also check for any test cases that might be testing error conditions
rg "def test.*" tests/test_vasp_poscar_dump.py tests/test_vasp_poscar_to_system.py
Length of output: 409
| @@ -135,4 +150,5 @@ def from_labeled_system(self, file_name, begin=0, step=1, **kwargs): | |||
| vol = np.linalg.det(np.reshape(data["cells"][ii], [3, 3])) | |||
| data["virials"][ii] *= v_pref * vol | |||
| data = uniq_atom_names(data) | |||
| register_move_data(data) | |||
There was a problem hiding this comment.
💡 Codebase verification
Move flags are not supported in XML format
Based on the examination of the XML parsing implementation in dpdata/vasp/xml.py and the sample XML file, I can confirm that the XML format does not currently support move flags (selective dynamics). The analyze_vasprun function only extracts positions, cell parameters, energies, forces, and stresses from the XML file, with no handling of selective dynamics flags.
- The registration of move data in
from_labeled_systemshould be removed as it's not applicable for XML format - If selective dynamics support is needed for XML format, it would require implementing the extraction of these flags from the XML structure
🔗 Analysis chain
Verify move flag support in XML format.
While the registration is placed consistently, we should verify that the XML format supports move flags and that they are being properly extracted in dpdata.vasp.xml.analyze.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if XML parsing handles move flags
# Test: Search for move flag handling in xml.py
rg -A 5 "move|selective" "dpdata/vasp/xml.py"
Length of output: 244
Script:
#!/bin/bash
# Let's check for move flag handling in both xml.py and the test files
rg -A 5 "move|selective" "dpdata/vasp/"
# Also check test files specifically for XML move flag tests
rg -A 5 "xml.*move|move.*xml" "tests/"
# Check the actual implementation of from_labeled_system in vasp.py
ast-grep --pattern 'def from_labeled_system'
Length of output: 4755
Script:
#!/bin/bash
# Let's check the XML parsing implementation
ast-grep --pattern 'class VASPXMLFormat'
# Check if there are any XML test files that might contain move flags
fd -e xml . tests/
# Look for any move-related functionality in the XML format class
rg -A 5 "from_labeled_system|analyze_vasprun" "dpdata/vasp/xml.py"
Length of output: 403
Script:
#!/bin/bash
# Let's examine one of the test XML files to see if it contains selective dynamics
head -n 50 "tests/poscars/vasprun.h2o.md.xml"
# Check if there are any test cases related to selective dynamics in XML
rg -A 5 "selective.*xml|move.*xml" "tests/"
# Look at the implementation of analyze_vasprun in xml.py
cat "dpdata/vasp/xml.py"
Length of output: 6104
| if move is None and data.get("move", None) is not None and len(data["move"]) > 0: | ||
| move = data["move"][frame_idx] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the move flag check.
The current implementation checks both the existence and length of move data. This could be simplified.
Consider this more concise implementation:
-if move is None and data.get("move", None) is not None and len(data["move"]) > 0:
+if move is None and data.get("move", []):
move = data["move"][frame_idx]This achieves the same result but is more readable and handles empty lists naturally.
📝 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 move is None and data.get("move", None) is not None and len(data["move"]) > 0: | |
| move = data["move"][frame_idx] | |
| if move is None and data.get("move", []): | |
| move = data["move"][frame_idx] | |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
tests/test_vasp_poscar_to_system.py (2)
17-20: LGTM! Consider adding docstring for test clarity.The test correctly validates the move flags conversion using numpy array comparison.
Add a docstring to explain the test's purpose and the expected move flags format:
def test_move_flags(self): + """ + Verify move flags conversion from POSCAR selective dynamics. + Expected format: [[[x1,y1,z1], [x2,y2,z2]]] for 2 atoms with True/False flags + per coordinate. + """ expected = np.array([[[True, True, False], [False, False, False]]]) self.assertTrue(np.array_equal(self.system["move"], expected))
23-39: LGTM! Consider grouping error tests in a separate test class.The error test cases comprehensively cover invalid scenarios for move flags handling.
Consider grouping these error tests in a dedicated test class for better organization:
-class TestPOSCARCart(unittest.TestCase): +class TestPOSCARMoveFlags(unittest.TestCase): + """Test cases for handling invalid move flags in POSCAR files.""" def test_move_flags_error1(self): with self.assertRaisesRegex(RuntimeError, "Invalid move flags.*?"): dpdata.System().from_vasp_poscar(os.path.join("poscars", "POSCAR.oh.err1"))dpdata/vasp/poscar.py (2)
42-44: Improve error message clarityThe error message could be more specific about the expected format of move flags.
- f"Invalid move flags, should be 6 columns, got {tmp}" + f"Invalid move flags: expected 6 columns (3 coordinates + 3 T/F flags), got {len(tmp)} columns"
69-70: Simplify nested if statementsAs suggested by static analysis, combine the nested if statements for better readability.
- if not is_cartesian: - if lines[7][0] not in ["d", "D"]: + if not is_cartesian and lines[7][0] not in ["d", "D"]:🧰 Tools
🪛 Ruff
69-70: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
dpdata/abacus/scf.py (2)
302-303: Add validation for move array shape.The move array should be validated before appending to ensure consistent dimensions. Consider adding shape validation similar to other arrays in the function.
if imove is not None: + if len(imove) != 3: + raise ValueError(f"Move flag must have 3 components, got {len(imove)}") move.append(imove)Also applies to: 314-315
618-619: Consider improving move parameter documentation.While the documentation is good, it could be more specific about the expected shape and type of the move parameter.
Update the docstring to clarify:
- move : list of (list of list of bool), optional - List of the move flag of each xyz direction of each atom for each frame + move : list of (list of bool), optional + List of boolean flags (length 3) for each atom indicating whether the atom + can move in x, y, z directions during geometry optimization. Each inner list + must contain exactly 3 boolean values.Also applies to: 693-695
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- dpdata/abacus/scf.py (8 hunks)
- dpdata/vasp/poscar.py (5 hunks)
- tests/poscars/POSCAR.oh.err1 (1 hunks)
- tests/poscars/POSCAR.oh.err2 (1 hunks)
- tests/test_vasp_poscar_to_system.py (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/poscars/POSCAR.oh.err1
- tests/poscars/POSCAR.oh.err2
🧰 Additional context used
🪛 Ruff
dpdata/vasp/poscar.py
69-70: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
tests/test_vasp_poscar_to_system.py
22-22: Redefinition of unused
TestPOSCARCartfrom line 12(F811)
🔇 Additional comments (4)
dpdata/vasp/poscar.py (2)
8-14: Make move flag parsing case-insensitiveAs mentioned in a previous review, the function should handle lowercase flags for better robustness. Users might input lowercase 't' or 'f' for selective dynamics flags.
Line range hint
7-131: Implementation successfully adds move flags supportThe changes effectively implement the conversion of move flags between Abacus and VASP formats. The implementation is thorough, with proper error handling and validation. The code structure is clean and maintainable.
🧰 Tools
🪛 Ruff
69-70: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
dpdata/abacus/scf.py (2)
482-482: LGTM: Move flag handling in get_frame is correct.The implementation properly handles the move flags by adding a new axis to match the array dimensions of other data fields.
Also applies to: 515-516
568-568: LGTM: Consistent move flag handling in get_frame_from_stru.The implementation maintains consistency with get_frame in handling move flags.
Also applies to: 578-579
tests/test_vasp_poscar_to_system.py
Outdated
| def test_move_flags(self): | ||
| expected = np.array([[[True, True, False], [False, False, False]]]) | ||
| self.assertTrue(np.array_equal(self.system["move"], expected)) | ||
|
|
||
|
|
||
| class TestPOSCARCart(unittest.TestCase): | ||
| def test_move_flags_error1(self): | ||
| with self.assertRaisesRegex(RuntimeError, "Invalid move flags.*?"): | ||
| dpdata.System().from_vasp_poscar(os.path.join("poscars", "POSCAR.oh.err1")) | ||
|
|
||
| def test_move_flags_error2(self): | ||
| with self.assertRaisesRegex(RuntimeError, "Invalid move flag: a"): | ||
| dpdata.System().from_vasp_poscar(os.path.join("poscars", "POSCAR.oh.err2")) | ||
|
|
||
| def test_move_flags_error3(self): | ||
| system = dpdata.System().from_vasp_poscar( | ||
| os.path.join("poscars", "POSCAR.oh.c") | ||
| ) | ||
| system.data["move"] = np.array([[[True, True], [False, False]]]) | ||
| with self.assertRaisesRegex( | ||
| RuntimeError, "Invalid move flags:.*?should be a list of 3 bools" | ||
| ): | ||
| system.to_vasp_poscar("POSCAR.tmp.1") |
There was a problem hiding this comment.
💡 Codebase verification
Additional test coverage needed for Abacus move flags conversion
The verification reveals that:
- Abacus STRU files contain move flags (represented as
1 1 1in atomic positions) - There are no dedicated test cases for Abacus move flags conversion, while VASP has comprehensive tests
- The implementation in
dpdata/abacus/scf.pyshows move flags are supported but not tested
Required test coverage:
- Test reading move flags from Abacus STRU files
- Test writing move flags to Abacus STRU files
- Test conversion between Abacus and VASP move flags formats
- Test error cases for invalid move flags in Abacus format
🔗 Analysis chain
Verify test coverage for move flags conversion.
While the current tests cover VASP POSCAR move flags validation, we should ensure there are corresponding tests for Abacus move flags conversion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of Abacus move flags
# Test: Search for move flags tests in Abacus-related test files
echo "Searching for Abacus move flags tests..."
rg -l "test.*move.*flags?" "tests/test_abacus_*"
Length of output: 201
Script:
#!/bin/bash
# Let's first find all test files related to Abacus
echo "Finding Abacus-related test files..."
fd "test.*abacus.*\.py$" tests/
# Then search for move flags in all test files
echo -e "\nSearching for move flags tests in all test files..."
rg -l "test.*move.*flags?" tests/
# Let's also check the implementation files for move flags handling
echo -e "\nSearching for move flags handling in implementation files..."
rg -C 3 "move.*flags?" --type py
Length of output: 7247
Script:
#!/bin/bash
# Let's check the implementation of Abacus move flags conversion
echo "Checking Abacus move flags conversion implementation..."
rg -C 5 "move.*flags?" dpdata/system.py
# Let's also check if there are any Abacus STRU files with move flags in test data
echo -e "\nSearching for Abacus STRU files..."
fd "STRU" tests/
# And examine their content for move flags
echo -e "\nChecking STRU files content for move flags..."
find tests/ -name "STRU" -type f -exec sh -c 'echo "=== {} ==="; cat {}' \;
Length of output: 5157
🧰 Tools
🪛 Ruff
22-22: Redefinition of unused
TestPOSCARCartfrom line 12(F811)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/test_vasp_poscar_to_system.py (1)
17-20: Enhance test coverage for move flags.While the basic test is good, consider adding:
- Documentation explaining what this test case represents
- Additional test cases covering different move flag combinations
- Verification that the input POSCAR file matches the expected values
Add docstring and more test cases:
def test_move_flags(self): + """Test move flags are correctly read from POSCAR file. + + The test file POSCAR.oh.c contains: + - First atom: movable in x,y directions only + - Second atom: fixed in all directions + """ expected = np.array([[[True, True, False], [False, False, False]]]) self.assertTrue(np.array_equal(self.system["move"], expected)) +def test_move_flags_all_true(self): + """Test when all atoms are movable in all directions.""" + system = dpdata.System() + system.from_vasp_poscar(os.path.join("poscars", "POSCAR.oh.all_true")) + expected = np.array([[[True, True, True], [True, True, True]]]) + self.assertTrue(np.array_equal(system["move"], expected))dpdata/vasp/poscar.py (1)
69-70: Simplify nested if statementsThe nested if statements can be combined for better readability.
- if not is_cartesian: - if lines[7][0] not in ["d", "D"]: + if not is_cartesian and lines[7][0] not in ["d", "D"]:🧰 Tools
🪛 Ruff
69-70: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- dpdata/vasp/poscar.py (5 hunks)
- tests/test_vasp_poscar_to_system.py (1 hunks)
🧰 Additional context used
🪛 Ruff
dpdata/vasp/poscar.py
69-70: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
🔇 Additional comments (5)
tests/test_vasp_poscar_to_system.py (1)
17-40: Add test coverage for Abacus move flags conversion.While the VASP POSCAR move flags tests are well-implemented, the PR objectives mention converting move flags between Abacus and VASP. Additional test coverage is needed for:
- Reading move flags from Abacus STRU files
- Writing move flags to Abacus STRU files
- Converting move flags between Abacus and VASP formats
- Error cases for invalid move flags in Abacus format
dpdata/vasp/poscar.py (4)
8-14: Make move flag comparison case-insensitiveThe move flag comparison should be case-insensitive to handle both uppercase and lowercase input (e.g., 't', 'f', 'T', 'F').
38-44: LGTM! Clear error handling for move flags validationThe validation of move flags is thorough with a descriptive error message.
124-128: LGTM! Clear validation and formatting of move flagsThe code properly validates the move flags format and converts them to the expected VASP format.
112-114: Verify move flags sorting matches atom positionsThe code correctly sorts move flags to match atom positions using the same sorting index. However, let's verify this behavior is tested.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/test_vasp_poscar_to_system.py (2)
17-20: Add docstring and consider additional test cases.The test implementation is good, but could be enhanced with:
- A docstring explaining the test's purpose and expected behavior
- More descriptive variable names (e.g.,
expected_move_flags)- Additional test cases covering different move flag combinations
def test_move_flags(self): + """ + Test that move flags are correctly read from POSCAR file. + Expected format: [[[bool, bool, bool], ...]] for each atom's x,y,z movement. + """ - expected = np.array([[[True, True, False], [False, False, False]]]) + expected_move_flags = np.array([[[True, True, False], [False, False, False]]]) - self.assertTrue(np.array_equal(self.system["move"], expected)) + self.assertTrue(np.array_equal(self.system["move"], expected_move_flags))
30-47: Add docstrings to error test cases.The error test cases provide good coverage, but could be enhanced with:
- Docstrings explaining each error scenario
- More descriptive test data file names (e.g.,
POSCAR.invalid_move_flags.case1)def test_move_flags_error1(self): + """ + Test that an appropriate error is raised when POSCAR contains + invalid move flags format. + """ with self.assertRaisesRegex(RuntimeError, "Invalid move flags.*?"): dpdata.System().from_vasp_poscar(os.path.join("poscars", "POSCAR.oh.err1")) def test_move_flags_error2(self): + """ + Test that an appropriate error is raised when POSCAR contains + invalid move flag character (e.g., 'a' instead of T/F). + """ with self.assertRaisesRegex(RuntimeError, "Invalid move flag: a"): dpdata.System().from_vasp_poscar(os.path.join("poscars", "POSCAR.oh.err2")) def test_move_flags_error3(self): + """ + Test that an appropriate error is raised when move flags array + has incorrect dimensions (expecting 3 bools per atom). + """ system = dpdata.System().from_vasp_poscar( os.path.join("poscars", "POSCAR.oh.c") )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/test_vasp_poscar_to_system.py (1 hunks)
🔇 Additional comments (2)
tests/test_vasp_poscar_to_system.py (2)
22-29: LGTM! Well-structured test class setup.The implementation follows test best practices with proper setup and cleanup of test resources.
17-47: Additional test coverage needed for Abacus move flags.While the VASP POSCAR move flags tests are well implemented, the PR objective of supporting move flags conversion between Abacus and VASP requires additional test coverage:
- Test reading move flags from Abacus STRU files
- Test writing move flags to Abacus STRU files
- Test conversion between Abacus and VASP move flags formats
- Test error cases for invalid move flags in Abacus format
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
dpdata/plugins/abacus.py (1)
70-80: LGTM! Good choice registering with System instead of LabeledSystem.
The implementation correctly registers the move flags as a system property rather than a label. This is architecturally sound since movement constraints are inherent properties of the atomic system rather than external labels or observations.
The shape (NFRAMES, NATOMS, 3) appropriately captures x,y,z movement flags for each atom across all frames.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- dpdata/plugins/abacus.py (4 hunks)
🔇 Additional comments (3)
dpdata/plugins/abacus.py (3)
90-90: LGTM! Appropriate registration of move data for SCF calculations.
The registration is correctly placed after magnetic data registration and properly integrates with the existing data flow.
102-102: LGTM! Consistent implementation across format handlers.
The registration of move data is appropriately added to both MD and relaxation format handlers, maintaining consistency with the SCF implementation.
Also applies to: 114-114
70-80: Verify VASP compatibility with move data registration.
Since this PR aims to support move flag conversion between Abacus and VASP, we should verify that the VASP implementation uses compatible data structures.
✅ Verification successful
Let me gather more specific information about the VASP implementation.
Move flag implementation is consistent between Abacus and VASP
The verification shows that the move flag implementation is properly handled in both Abacus and VASP:
- Both implementations register the move data type with identical structure
(Axis.NFRAMES, Axis.NATOMS, 3) - VASP implementation includes proper move flag parsing and validation
- Test coverage exists for move flags in VASP implementation
- The data structures are compatible between both formats
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for corresponding VASP implementation and data structure compatibility
# Look for similar move data registration in VASP-related files
echo "Searching for move data handling in VASP files..."
rg -l "move" --type py | grep -i "vasp"
# Check for DataType registration patterns similar to the Abacus implementation
echo "Checking for similar DataType registrations..."
ast-grep --pattern 'DataType($_, $_, (Axis.NFRAMES, Axis.NATOMS, 3), $_)'
Length of output: 793
Script:
#!/bin/bash
# Check VASP implementation for move flag handling
echo "Checking VASP implementation for move flags..."
rg -A 5 -B 5 "move" dpdata/plugins/vasp.py
# Check test files for move-related test cases
echo "Checking test files for move flag tests..."
rg -A 5 -B 5 "move" tests/test_vasp_poscar_to_system.py tests/test_vasp_poscar_dump.py
# Check POSCAR handling
echo "Checking POSCAR handling for move flags..."
rg -A 5 -B 5 "move" dpdata/vasp/poscar.py
Length of output: 9709
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Head branch was pushed to by a user without write access
Angel-Jia
force-pushed
the
yaosk/atoms_flag
branch
from
2319b33
to
dec6350
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
dpdata/plugins/abacus.py (1)
70-80: Good implementation of move data registration!
The implementation correctly registers the move flags as a DataType with appropriate dimensionality (frames × atoms × 3D). The registration with dpdata.System instead of dpdata.LabeledSystem is the right choice since movement flags are structural data rather than labels/properties.
Consider documenting the meaning of the move flags in the docstring, especially the interpretation of the 3D vector values, to help future maintainers understand the data structure.
dpdata/vasp/poscar.py (3)
42-44: Improve error message for invalid move flags
The error message should be more descriptive about the expected format.
Apply this diff:
raise RuntimeError(
- f"Invalid move flags, should be 6 columns, got {tmp}"
+ f"Invalid selective dynamics format: expected 6 columns (3 coordinates + 3 T/F flags), got {len(tmp)} columns"
)69-70: Simplify nested if statements
Combine the nested if statements for better readability.
Apply this diff:
- if not is_cartesian:
- if lines[7][0] not in ["d", "D"]:
+ if not is_cartesian and lines[7][0] not in ["d", "D"]:🧰 Tools
🪛 Ruff
69-70: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
Line range hint 99-131: LGTM with minor simplification suggestion
The move flags handling is implemented correctly. Consider simplifying the move flags validation and formatting.
Apply this diff to simplify the code:
if move is not None and len(move) > 0:
move = move[f_idx][sort_idx]
if isinstance(move, np.ndarray):
move = move.tolist()
posi_list = []
for idx in range(len(posis)):
ii_posi = posis[idx]
line = f"{ii_posi[0]:15.10f} {ii_posi[1]:15.10f} {ii_posi[2]:15.10f}"
if move is not None and len(move) > 0:
- move_flags = move[idx]
- if not isinstance(move_flags, list) or len(move_flags) != 3:
- raise RuntimeError(
- f"Invalid move flags: {move_flags}, should be a list of 3 bools"
- )
- line += " " + " ".join("T" if flag else "F" for flag in move_flags)
+ try:
+ move_flags = move[idx]
+ if len(move_flags) != 3:
+ raise ValueError
+ line += " " + " ".join("T" if flag else "F" for flag in move_flags)
+ except (TypeError, ValueError):
+ raise RuntimeError(
+ f"Invalid move flags at index {idx}: {move_flags}, should be a list of 3 bools"
+ )dpdata/plugins/vasp.py (1)
18-28: Add docstring to document the function's purpose and parameters.
The implementation looks good! The function correctly handles the registration of move data with appropriate dimensions and optional flag.
Consider adding a docstring to document the function:
def register_move_data(data):
+ """Register move data type if present in the data dictionary.
+
+ Parameters
+ ----------
+ data : dict
+ Dictionary containing system data, may include 'move' key
+ """
if "move" in data:
dt = DataType(dpdata/abacus/scf.py (2)
515-516: Consider adding shape validation for move data.
While the move data is correctly added to the frame, consider validating its shape before adding it to ensure consistency with the frame's dimensions.
if len(move) > 0:
+ expected_shape = (len(types), 3)
+ if move.shape != expected_shape:
+ warnings.warn(f"Unexpected move shape: {move.shape}, expected: {expected_shape}")
data["move"] = move[np.newaxis, :, :]578-579: Maintain consistency with move data shape validation.
For consistency with the previous suggestion, consider adding the same shape validation here.
if len(move) > 0:
+ expected_shape = (len(types), 3)
+ if move.shape != expected_shape:
+ warnings.warn(f"Unexpected move shape: {move.shape}, expected: {expected_shape}")
data["move"] = move[np.newaxis, :, :]📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- dpdata/abacus/md.py (2 hunks)
- dpdata/abacus/relax.py (2 hunks)
- dpdata/abacus/scf.py (8 hunks)
- dpdata/plugins/abacus.py (4 hunks)
- dpdata/plugins/vasp.py (4 hunks)
- dpdata/vasp/poscar.py (5 hunks)
- tests/abacus.scf/STRU.ch4 (1 hunks)
- tests/abacus.scf/stru_test (1 hunks)
- tests/poscars/POSCAR.oh.err1 (1 hunks)
- tests/poscars/POSCAR.oh.err2 (1 hunks)
- tests/test_abacus_stru_dump.py (1 hunks)
- tests/test_vasp_poscar_dump.py (1 hunks)
- tests/test_vasp_poscar_to_system.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- dpdata/abacus/md.py
- dpdata/abacus/relax.py
- tests/abacus.scf/STRU.ch4
- tests/abacus.scf/stru_test
- tests/poscars/POSCAR.oh.err1
- tests/poscars/POSCAR.oh.err2
- tests/test_abacus_stru_dump.py
- tests/test_vasp_poscar_dump.py
- tests/test_vasp_poscar_to_system.py
🧰 Additional context used
🪛 Ruff
dpdata/vasp/poscar.py
69-70: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🔇 Additional comments (7)
dpdata/plugins/abacus.py (1)
90-90: Verify move data handling in format-specific get_frame functions.
The registration of move data is consistently added across all three formats (SCF, MD, Relax). However, we should verify that the corresponding get_frame functions in their respective modules properly handle the move data.
Let's check the implementation in the related files:
Also applies to: 102-102, 114-114
✅ Verification successful
Move data is properly handled in all ABACUS format implementations
The verification confirms that move data is correctly handled across all ABACUS formats:
- In
scf.py, move data is extracted from STRU file viaget_coords()and stored indata["move"] - In
relax.py, move data is obtained similarly and properly registered - In
md.py, move data follows the same pattern of extraction and registration
The consistent implementation across all three formats ensures proper move data handling before register_move_data is called.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if get_frame functions handle move data in the format-specific modules
echo "Checking get_frame implementations for move data handling:"
rg -A 5 "def get_frame" "dpdata/abacus/(scf|md|relax).py"
Length of output: 258
Script:
#!/bin/bash
# Let's first find the correct paths for the ABACUS format files
fd "scf.py|md.py|relax.py" --full-path
# Then check get_frame implementations
echo "Checking get_frame implementations:"
rg -A 10 "def get_frame"
Length of output: 6000
Script:
#!/bin/bash
# Let's check the implementation of get_coords function which seems to handle move data
echo "Checking get_coords implementation in ABACUS:"
rg -A 15 "def get_coords" dpdata/abacus/
# Also check where move data is used
echo -e "\nChecking move data usage:"
rg "move.*=" dpdata/abacus/
Length of output: 4541
dpdata/plugins/vasp.py (3)
41-41: LGTM! Appropriate integration for POSCAR format.
The placement of register_move_data after data processing is correct, as POSCAR format explicitly supports selective dynamics flags.
116-116: LGTM! Appropriate integration for OUTCAR format.
The placement of register_move_data after data processing is correct, as OUTCAR format includes atomic position constraints.
153-153:
Remove move data registration for XML format.
As noted in previous reviews and confirmed by examining the XML parsing implementation, the XML format does not currently support move flags. The analyze function in dpdata.vasp.xml does not extract selective dynamics information.
Remove this line as it's not applicable for the XML format. If move flag support is needed for XML format in the future, it would require:
- Implementing the extraction of selective dynamics flags in
dpdata.vasp.xml.analyze - Adding this registration only after such implementation
- register_move_data(data)dpdata/abacus/scf.py (3)
Line range hint 302-315: LGTM: Move flag handling is properly implemented.
The implementation correctly handles the move flags by:
- Conditionally appending move flags when present
- Converting the array to boolean type for consistency
693-695: Past review comment is still applicable.
The condition for handling move data was previously reviewed and a suggestion for simplification was made.
Line range hint 1-1: Consider adding test coverage for move flag handling.
While the implementation looks solid, it would be beneficial to add comprehensive test cases for:
- Reading move flags from STRU files
- Writing move flags to STRU files
- Edge cases like invalid move flag formats
Would you like me to help create test cases for the move flag functionality?
| def move_flag_mapper(flag): | ||
| if flag == "T": | ||
| return True | ||
| elif flag == "F": | ||
| return False | ||
| else: | ||
| raise RuntimeError(f"Invalid move flag: {flag}") |
There was a problem hiding this comment.
Make move flag parsing case-insensitive
The function should handle both uppercase and lowercase flags as both are commonly used in VASP files.
Apply this diff:
def move_flag_mapper(flag):
- if flag == "T":
+ if flag.upper() == "T":
return True
- elif flag == "F":
+ elif flag.upper() == "F":
return False
else:
raise RuntimeError(f"Invalid move flag: {flag}")📝 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.
| def move_flag_mapper(flag): | |
| if flag == "T": | |
| return True | |
| elif flag == "F": | |
| return False | |
| else: | |
| raise RuntimeError(f"Invalid move flag: {flag}") | |
| def move_flag_mapper(flag): | |
| if flag.upper() == "T": | |
| return True | |
| elif flag.upper() == "F": | |
| return False | |
| else: | |
| raise RuntimeError(f"Invalid move flag: {flag}") |
Summary by CodeRabbit
Release Notes
New Features
moveparameter across multiple functions, enabling more detailed tracking of atom movements.Bug Fixes
Tests