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

API: Build expressions with sympy directly, remove expressions.py #14

Open
nawtrey opened this issue Jul 28, 2020 · 3 comments
Open

API: Build expressions with sympy directly, remove expressions.py #14

nawtrey opened this issue Jul 28, 2020 · 3 comments
Assignees
Labels
API API-related changes enhancement New feature or request

Comments

@nawtrey
Copy link
Collaborator

nawtrey commented Jul 28, 2020

The idea is to use the new precedent set by kda.calculate_thermo_force() of incorporating the string-to-SymPy conversions into the calculation functions themselves instead of having separate functions for this. The original idea was to give the user the maximum amount of flexibility possible, but now it seems giving access to the string versions of functions doesn't really add any value. It would be simpler from a user's perspective to be given the SymPy functions that can then be substituted/simplified and converted to lambda functions.

@nawtrey nawtrey added the enhancement New feature or request label Jul 28, 2020
@nawtrey nawtrey self-assigned this Jul 28, 2020
@nawtrey
Copy link
Collaborator Author

nawtrey commented Aug 20, 2020

Another reason to do this is that it is not straightforward for a user to go from a set of string functions to the final SymPy function they are looking for, but it is easy to go from a SymPy function back to a string. Simply cast any SymPy function as a string and you will get back your string function.

@nawtrey
Copy link
Collaborator Author

nawtrey commented Mar 14, 2024

Perhaps a more ambitious goal would be to handle all the expression creation with sympy (instead of constructing expressions as strings and parsing them with sympy.parser.parse_expr()). This is actually what they recommend in their "best practices". My only concern would be an increase in run time or memory usage, but I think it would be easy to make the change and compare.

@nawtrey nawtrey changed the title Remove string to SymPy functions API: Build expressions with sympy directly, remove expressions.py Aug 8, 2024
@nawtrey nawtrey added the API API-related changes label Aug 8, 2024
@nawtrey
Copy link
Collaborator Author

nawtrey commented Aug 8, 2024

This is actually a simple conversion in terms of code and only comes at a small cost to performance. There are some key benefits to doing this, some of which are highlighted by the SymPy folks themselves (see here).

I don't think I need to provide much motivation here, but basically we should be building our expressions in terms of sympy.symbols from the very beginning. This is the pythonic way to build algebraic expressions (they discuss some pitfalls of using parse_expr on string functions) and allows us to enforce assumptions about our parameters (i.e. k12, k21 = symbols('k_{12} k_{21}', real=True, positive=True)). There are some smaller benefits, like the ability to retrieve the list of variables from an expression very easily.

Once converted, we can likely remove the entire expressions.py module since it will be completely unnecessary. Even longer term it would be nice to switch everything to use symbolic=True as the default, and remove all numeric calculations from our code. Numeric evaluations can always be performed on KDA expressions once they have been lambified, and that should be the encouraged procedure if numerical outputs are needed.

Unfortunately we will have to wait to implement these changes to make sure master is able to run the kda manuscript code for some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API-related changes enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant