parse multiple epsilon values for lj/cut/soft#88
parse multiple epsilon values for lj/cut/soft#88Yi-FanLi merged 12 commits intodeepmodeling:develfrom
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe pull request introduces a new function, Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 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 (
|
![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 (1)
dpti/hti_liq.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
dpti/hti_liq.py
50-55: .format call is missing argument(s) for placeholder(s): p
(F524)
🔇 Additional comments (7)
dpti/hti_liq.py (7)
76-76: LGTM: Good refactoring to use the new function.The refactoring simplifies the code by using the new centralized function for parameter processing.
79-80: Compute and variable definitions align with the refactoring.The compute definition for
lj_peand the variable definition fore_diffare properly updated to match the new parameter processing approach.
98-98: LGTM: Consistent use of the refactored function.Properly using the same centralized function for the hybrid case.
108-108: LGTM: Consistent variable assignment.The variable assignment for
e_diffis correctly defined using the compute array element.
126-126: LGTM: Consistent application of the refactoring pattern.The refactoring pattern is consistently applied across all force field setup functions.
129-130: Compute and variable definitions align with the refactoring.The compute and variable definitions are properly updated to match the parameter handling approach.
197-197: LGTM: Updated thermo_style to use variable instead of compute.The thermo_style is updated to use
v_e_diffinstead ofc_e_diff[1], which aligns with the variable-based approach in the refactored code.
| def parse_lj_sigma_epsilon(ret, sparam, hybrid=False): | ||
| element_num = sparam.get("element_num", 1) | ||
| sigma_key_index = filter( | ||
| lambda t: t[0] <= t[1], | ||
| ((i, j) for i in range(element_num) for j in range(element_num)), | ||
| ) | ||
| activation = sparam["activation"] | ||
| epsilon = sparam.get("epsilon", None) | ||
| epsilon_0_0 = sparam.get("epsilon_0_0", None) | ||
| if hybrid: | ||
| pair_coeff_str = "lj/cut/soft " | ||
| else: | ||
| pair_coeff_str = "" | ||
| if epsilon is not None: | ||
| assert ( | ||
| epsilon_0_0 is None | ||
| ), "epsilon and epsilon_0_0 cannot be set at the same time" | ||
| ret += f"variable EPSILON equal {epsilon:f}\n" | ||
| for i, j in sigma_key_index: | ||
| ret += "pair_coeff {} {} {p:s}${{EPSILON}} {:f} {:f}\n".format( | ||
| i + 1, | ||
| j + 1, | ||
| pair_coeff_str, | ||
| sparam["sigma_" + str(i) + "_" + str(j)], | ||
| ) | ||
| else: | ||
| assert epsilon_0_0 is not None, "epsilon or epsilon_0_0 must be set" | ||
| for i, j in sigma_key_index: | ||
| ret += "pair_coeff {} {} {:s}{:f} {:f} {:f}\n".format( | ||
| i + 1, | ||
| j + 1, | ||
| pair_coeff_str, | ||
| sparam["epsilon_" + str(i) + "_" + str(j)], | ||
| sparam["sigma_" + str(i) + "_" + str(j)], | ||
| activation, | ||
| ) | ||
| return ret |
There was a problem hiding this comment.
The new function centralizes parameter processing nicely, but contains a formatting bug.
This new function appropriately centralizes the logic for processing Lennard-Jones parameters, making the code more maintainable. However, there's a formatting error in the string template.
In line 50, the string contains a {p:s} placeholder, but no corresponding p value is provided in the .format() call. Fix it like this:
- ret += "pair_coeff {} {} {p:s}${{EPSILON}} {:f} {:f}\n".format(
+ ret += "pair_coeff {} {} {}${{EPSILON}} {:f} {:f}\n".format(
i + 1,
j + 1,
pair_coeff_str,
sparam["sigma_" + str(i) + "_" + str(j)],
+ activation,
)📝 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 parse_lj_sigma_epsilon(ret, sparam, hybrid=False): | |
| element_num = sparam.get("element_num", 1) | |
| sigma_key_index = filter( | |
| lambda t: t[0] <= t[1], | |
| ((i, j) for i in range(element_num) for j in range(element_num)), | |
| ) | |
| activation = sparam["activation"] | |
| epsilon = sparam.get("epsilon", None) | |
| epsilon_0_0 = sparam.get("epsilon_0_0", None) | |
| if hybrid: | |
| pair_coeff_str = "lj/cut/soft " | |
| else: | |
| pair_coeff_str = "" | |
| if epsilon is not None: | |
| assert ( | |
| epsilon_0_0 is None | |
| ), "epsilon and epsilon_0_0 cannot be set at the same time" | |
| ret += f"variable EPSILON equal {epsilon:f}\n" | |
| for i, j in sigma_key_index: | |
| ret += "pair_coeff {} {} {p:s}${{EPSILON}} {:f} {:f}\n".format( | |
| i + 1, | |
| j + 1, | |
| pair_coeff_str, | |
| sparam["sigma_" + str(i) + "_" + str(j)], | |
| ) | |
| else: | |
| assert epsilon_0_0 is not None, "epsilon or epsilon_0_0 must be set" | |
| for i, j in sigma_key_index: | |
| ret += "pair_coeff {} {} {:s}{:f} {:f} {:f}\n".format( | |
| i + 1, | |
| j + 1, | |
| pair_coeff_str, | |
| sparam["epsilon_" + str(i) + "_" + str(j)], | |
| sparam["sigma_" + str(i) + "_" + str(j)], | |
| activation, | |
| ) | |
| return ret | |
| def parse_lj_sigma_epsilon(ret, sparam, hybrid=False): | |
| element_num = sparam.get("element_num", 1) | |
| sigma_key_index = filter( | |
| lambda t: t[0] <= t[1], | |
| ((i, j) for i in range(element_num) for j in range(element_num)), | |
| ) | |
| activation = sparam["activation"] | |
| epsilon = sparam.get("epsilon", None) | |
| epsilon_0_0 = sparam.get("epsilon_0_0", None) | |
| if hybrid: | |
| pair_coeff_str = "lj/cut/soft " | |
| else: | |
| pair_coeff_str = "" | |
| if epsilon is not None: | |
| assert ( | |
| epsilon_0_0 is None | |
| ), "epsilon and epsilon_0_0 cannot be set at the same time" | |
| ret += f"variable EPSILON equal {epsilon:f}\n" | |
| for i, j in sigma_key_index: | |
| ret += "pair_coeff {} {} {}${{EPSILON}} {:f} {:f}\n".format( | |
| i + 1, | |
| j + 1, | |
| pair_coeff_str, | |
| sparam["sigma_" + str(i) + "_" + str(j)], | |
| activation, | |
| ) | |
| else: | |
| assert epsilon_0_0 is not None, "epsilon or epsilon_0_0 must be set" | |
| for i, j in sigma_key_index: | |
| ret += "pair_coeff {} {} {:s}{:f} {:f} {:f}\n".format( | |
| i + 1, | |
| j + 1, | |
| pair_coeff_str, | |
| sparam["epsilon_" + str(i) + "_" + str(j)], | |
| sparam["sigma_" + str(i) + "_" + str(j)], | |
| activation, | |
| ) | |
| return ret |
🧰 Tools
🪛 Ruff (0.8.2)
50-55: .format call is missing argument(s) for placeholder(s): p
(F524)
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
* parse multiple epsilon values for lj/cut/soft * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * correct pair_coeff for hybrid style * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * support multiple activation values * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * support using a single sigma to set sigma for all pairs * add an example for hti_liq of 2 atomic types * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
1 participant
This PR supports parsing pair-specific
epsilonvalues (epsilon_0_0,epsilon_0_1,epsilon_1_1, etc) for multiple types. This feature can be useful for atomic liquids with multiple atom types.Summary by CodeRabbit
New Features
Refactor