Skip to content
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

Sympy potential #5019

Merged
merged 2 commits into from
Mar 6, 2025
Merged

Sympy potential #5019

merged 2 commits into from
Mar 6, 2025

Conversation

ludogibbs
Copy link
Contributor

@ludogibbs ludogibbs commented Dec 16, 2024

Description of changes:

  • new feature: generate non-bonded tabulated potentials from sympy expressions

@RudolfWeeber
Copy link
Contributor

Thanks, Lukas. Plesae address the following:

  • Add your test case to the CMakeList.txt in testsuite/python, so it gets actually run in CI.
  • translate the comments in your code to English
  • pls use a more meaningful arument name than f for the energy expression
  • I like the string-based expression input, but it does not cover all cases (e.g. piecewhise functions or other more unusual sympy expressions). So please keep that but also accept a sympy expression in additoin to a string for the argument.
  • A furhter argument distance_symbol should be added, so people can use something else than r. Keeping r as default is fine, I guess.

Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Don't forget to add the test to the CMake build system and to install and run the linters on your workstation. Here are the relevant sections in the developer guide:
Linter and code formatter, Python integration tests.

@RudolfWeeber
Copy link
Contributor

Looks good. Just 2 things:

  • please rename the f argument to sth more meaningfull likle energy_expr
  • the linter is complaining about the test importing something fro mSympy you aren't using.

@RudolfWeeber
Copy link
Contributor

@jngrad could you pls take a look? I am not sure, what the linter wants.
Once that's resoled, we can go ahead with this pr, form my side.

jngrad and others added 2 commits March 6, 2025 20:43
@jngrad jngrad force-pushed the sympy_potential branch from 9fe6f7b to 60e821f Compare March 6, 2025 20:40
@jngrad jngrad added this to the ESPResSo 4.3.0 milestone Mar 6, 2025
@jngrad jngrad added New Feature automerge Merge with kodiak labels Mar 6, 2025
@kodiakhq kodiakhq bot merged commit af376c1 into espressomd:python Mar 6, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge with kodiak New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants