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

Control automatic function parameter interpolation method #2410

Closed
Scottmar93 opened this issue Oct 28, 2022 · 5 comments · Fixed by #2510
Closed

Control automatic function parameter interpolation method #2410

Scottmar93 opened this issue Oct 28, 2022 · 5 comments · Fixed by #2510
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours feature

Comments

@Scottmar93
Copy link
Contributor

Description

Allow users to specify which interpolation method they would like to be applied to function parameters.

Motivation

When using three-dimensional lookup tables I have found better model build times when moving from "cubic" to "linear" interpolation (in particular convert_to_casadi is slow). At the moment I can only control this by modifying the process symbol method of parameter values.

The speed up might be due to differences in calculating the Jacobian (or perhaps not calculating it if the Jacobian at all is not defined everywhere for linear interpolation) but I am not sure of the exact reason.

Possible Implementation

I can think of a couple of ways of doing this but I'm not sure what is preferable for everyone. Both should be non-breaking.

1. Pass optional dictionary to parameter_values class
This would just involve adding an optional argument to the parameter_values class and then adding the input as an attribute of the class. The user would just pass an options dictionary in the following way:

options = {"interpolation methods": {"name of function parameter to target": "linear"} 

if not specified then "cubic" interpolation will be used so that this change is nonbreaking.

2. Allow users to add a third entry to the tuple within the parameter values dictionary
i.e.

options = {"interpolation method": "linear"}
parameter_values_dictionary = {
   "negative electrode open circuit voltage [V]": ("negative electrode open circuit voltage [V]", ( (x_data, ), y_data), options), 
   "positive electrode open circuit voltage [V]": ("positive electrode open circuit voltage [V]", ( (x_data, ), y_data)), 
}

This could either be a dictionary like I suggest here (so that additional control can be easily added later) or it could just be "linear" or "cubic". This third argument should be optional with "cubic" used by default so that we don't break all previous code.

Additional context

I am happy to implement once there is agreement on how to do so.

Could perhaps be done in conjunction with modifications to function parameter in #2403

@Scottmar93 Scottmar93 added feature difficulty: easy A good issue for someone new. Can be done in a few hours labels Oct 28, 2022
@rtimms
Copy link
Contributor

rtimms commented Oct 28, 2022

You can also create the Interpolant first to give more control rather than passing data directly, but I think a solution like you suggest would be useful.

@valentinsulzer
Copy link
Member

Passing in the interpolant directly might be tricky because you don't know what the children objects are from outside the model classes. However, you can create a function that returns the interpolant object. I would favor this route (examples demonstrating how to do this in 1D, 2D, 3D) rather than functionality that lets you pass options, since it's fairly easy to do yourself if there is an example

It's interesting that you found linear to be better than cubic. I thought cubic worked better because of needing to differentiate? Ideally, the default would be linear.

@rtimms
Copy link
Contributor

rtimms commented Oct 28, 2022

Ah yeah good point. When I changed the default interpolant to linear I left the function parameters as cubic as I thought differentiating would lead to issues. I agree having linear as the default everywhere would be best.

@Scottmar93
Copy link
Contributor Author

To be specific solve time doesn't really change for the case I considered but model setup time does:

  • linear: 52ms setup, 2.78s solve time
  • cubic: 23s setup, 2.9s solve time

Should I go ahead with changing to linear? Worry would be that this might alter some results.

However, you can create a function that returns the interpolant object. I would favor this route (examples demonstrating how to do this in 1D, 2D, 3D) rather than functionality that lets you pass options, since it's fairly easy to do yourself if there is an example

Yeah agree that this is a way to do this within a script. I was trying to keep parameter definitions within a parameters class (like LithiumIonParameters) and data loading separate but I guess can just pass data to the parameters class an create Interpolants instead of FunctionParameters

@rtimms
Copy link
Contributor

rtimms commented Nov 1, 2022

Would be interesting to do a bit more detailed profiling to see where the bottleneck is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants