Fix velocity when resetting structure #356
Conversation
WalkthroughThe change updates the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## main #356 +/- ## ========================================== + Coverage 81.12% 81.22% +0.10% ========================================== Files 5 5 Lines 551 554 +3 ========================================== + Hits 447 450 +3 Misses 104 104 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pylammpsmpi/wrapper/ase.py(3 hunks)
🔇 Additional comments (2)
pylammpsmpi/wrapper/ase.py (2)
312-312: LGTM!Correctly passes the computed velocities variable instead of hardcoded None.
322-322: LGTM!Correctly passes the computed velocities variable for the multi-core case, maintaining consistency with the single-core implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_ase_interface.py (1)
161-163: Consider usingnp.allclosefor velocity comparison.The test uses
np.equalfor velocity comparison, which requires exact equality. Given that velocities might undergo coordinate transformations in non-orthogonal cells,np.allclosewould be more robust against floating-point precision issues.- self.assertTrue( - np.all(np.equal(lmp.interactive_velocities_getter(), velocities_initial)) - ) + self.assertTrue( + np.all(np.allclose(lmp.interactive_velocities_getter(), velocities_initial)) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_ase_interface.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_ase_interface.py (1)
pylammpsmpi/wrapper/ase.py (7)
LammpsASELibrary(16-469)interactive_structure_setter(192-329)interactive_lib_command(66-75)interactive_positions_getter(77-90)interactive_energy_pot_getter(340-347)interactive_velocities_getter(441-448)interactive_positions_setter(92-108)
🔇 Additional comments (3)
tests/test_ase_interface.py (3)
124-138: LGTM! Test setup follows established patterns.The test initialization correctly follows the same pattern as other tests in the class, using appropriate logger configuration and LammpsLibrary backend.
164-174: LGTM! Position modification test is well-structured.The test correctly modifies positions, updates LAMMPS, and validates both energy and position changes. This ensures the velocity functionality doesn't interfere with position handling.
135-138: Verify velocity array length matches bulk Al structure atom countWe’re hardcoding four 3-component velocity vectors but couldn’t import ASE here to confirm that
bulk('Al', cubic=True)indeed yields exactly 4 atoms. Please manually verify that the structure has 4 atoms (i.e. positions shape(4, 3)), and if it differs, updatevelocities_initialaccordingly.• File:
tests/test_ase_interface.py(lines 135–138)
• Snippet:velocities_initial = np.array( [[1.0, 1.0, 1.0], [1.0, 1.0, 1.0], [1.0, 1.0, 1.0], [1.0, 1.0, 1.0]] ) structure.set_velocities(velocities_initial)
| def test_velocities(self): | ||
| lmp = LammpsASELibrary( | ||
| working_directory=None, | ||
| cores=1, | ||
| comm=None, | ||
| logger=logging.getLogger("TestStaticLogger"), | ||
| log_file=None, | ||
| library=LammpsLibrary(cores=2), | ||
| disable_log_file=True, | ||
| ) | ||
| structure = bulk("Al", cubic=True) | ||
| velocities_initial = np.array( | ||
| [[1.0, 1.0, 1.0], [1.0, 1.0, 1.0], [1.0, 1.0, 1.0], [1.0, 1.0, 1.0]] | ||
| ) | ||
| structure.set_velocities(velocities_initial) | ||
| lmp.interactive_structure_setter( | ||
| structure=structure, | ||
| units="lj", | ||
| dimension=3, | ||
| boundary=" ".join(["p" if coord else "f" for coord in structure.pbc]), | ||
| atom_style="atomic", | ||
| el_eam_lst=["Al"], | ||
| calc_md=False, | ||
| ) | ||
| lmp.interactive_lib_command("pair_style lj/cut 6.0") | ||
| lmp.interactive_lib_command("pair_coeff 1 1 1.0 1.0 4.04") | ||
| lmp.interactive_lib_command( | ||
| command="thermo_style custom step temp pe etotal pxx pxy pxz pyy pyz pzz vol" | ||
| ) | ||
| lmp.interactive_lib_command(command="thermo_modify format float %20.15g") | ||
| lmp.interactive_lib_command("run 0") | ||
| self.assertTrue( | ||
| np.all(np.isclose(lmp.interactive_positions_getter(), structure.positions)) | ||
| ) | ||
| self.assertTrue( | ||
| np.isclose(lmp.interactive_energy_pot_getter(), -0.04342932384411341) | ||
| ) | ||
| self.assertTrue( | ||
| np.all(np.equal(lmp.interactive_velocities_getter(), velocities_initial)) | ||
| ) | ||
| positions = structure.positions.copy() | ||
| positions[0] += np.array([0.1, 0.1, 0.1]) | ||
| lmp.interactive_positions_setter(positions=positions) | ||
| lmp.interactive_lib_command("run 0") | ||
| self.assertTrue( | ||
| np.isclose(lmp.interactive_energy_pot_getter(), -0.043829529274767506) | ||
| ) | ||
| self.assertTrue( | ||
| np.all(np.isclose(lmp.interactive_positions_getter(), positions)) | ||
| ) | ||
| |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test coverage gaps: Missing velocity persistence and edge cases.
The test validates initial velocity setting but doesn't verify velocity persistence after position updates or test edge cases like zero velocities or missing velocity data.
Consider adding these test scenarios:
# After line 173, add velocity persistence check: self.assertTrue( np.all(np.allclose(lmp.interactive_velocities_getter(), velocities_initial)), "Velocities should persist after position updates" ) # Also consider testing with zero velocities or no initial velocities🤖 Prompt for AI Agents
In tests/test_ase_interface.py around lines 124 to 174, the test_velocities method sets initial velocities but does not verify if velocities persist after updating positions or handle edge cases like zero or missing velocities. To fix this, add an assertion after updating positions to check that velocities remain unchanged using np.allclose on lmp.interactive_velocities_getter() and velocities_initial. Additionally, create new test cases to set zero velocities and test behavior when velocities are not set initially, ensuring these edge cases are covered. 
* Implement _vector_to_lammps * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * run black * Remove flatten because it's always flattened --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Summary by CodeRabbit