-
Notifications
You must be signed in to change notification settings - Fork 4
LAMMPS: Add option to couple xyz for npt #609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughA single-file enhancement adds a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/atomistics/calculators/lammps/libcalculator.py (2)
481-481: Consider consistency with NPT function.The implementation mirrors the pattern from
calc_molecular_dynamics_npt_with_lammpslib, which is good for consistency. The same variable naming suggestion applies here: consider using lowercaselammps_ensemble_npt_xyzinstead of ALL_CAPS.The conditional logic (lines 496-499) is duplicated from lines 241-244. While extracting it to a helper function could reduce duplication, with only two occurrences and 4 lines each, this is acceptable as-is.
Also applies to: 496-500
236-244: The LAMMPS syntax is correct, but consider lowercase naming for local variables.The implementation correctly appends
" couple xyz"to the ensemble command. LAMMPS documentation confirms this is the proper syntax for coupling NPT ensemble dimensions.However, the variable
LAMMPS_ENSEMBLE_NPT_XYZuses ALL_CAPS naming typically reserved for module-level constants. Since this is a locally scoped variable, consider renaming it toensemble_npt_xyzor similar lowercase format to follow Python conventions.This pattern repeats in the second function as well.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/atomistics/calculators/lammps/libcalculator.py
🧰 Additional context used
🪛 Ruff (0.14.10)
src/atomistics/calculators/lammps/libcalculator.py
238-238: Do not perform function call OutputMolecularDynamics.keys in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.14)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.13)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11)
- GitHub Check: notebooks
- GitHub Check: unittest_grace
- GitHub Check: unittest_mace
- GitHub Check: coverage
- GitHub Check: unittest_old
- GitHub Check: unittest_qe
- GitHub Check: unittest_siesta
- GitHub Check: unittest_matrix (macos-latest, 3.14)
- GitHub Check: unittest_matrix (windows-latest, 3.14)
- GitHub Check: unittest_sphinxdft
- GitHub Check: unittest_gpaw
- GitHub Check: unittest_abinit
- GitHub Check: minimal
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #609 +/- ##
==========================================
- Coverage 86.81% 86.76% -0.05%
==========================================
Files 43 43
Lines 2419 2425 +6
==========================================
+ Hits 2100 2104 +4
- Misses 319 321 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.