add warning to klu if using model converted to casadi

[rtimms]

the IDAKLUSolver can be considerably slower when using model.convert_to_format = "casadi" compared with model.convert_to_format = "python", we should add a warning and/or explain in the docs for the solver that we recommend converting to python for this solver

[Saransh-cpp]

@tinosulzer is this issue supposed to be solved using the code added by you in the commit above?

[valentinsulzer]

Yes, but I was also getting errors when trying to use the "python" format. Might be fixed now that #1359 is merged

[Saransh-cpp]

Yes, I ran the tests after adding the code and it went fine. I'll make the necessary changes and create a PR!

[valentinsulzer]

There is still a bug in the jacobian calculation (when not converting to casadi), the following should work but gives an error:

#
# Example showing how to load and solve the DFN
#

import pybamm
import numpy as np

pybamm.set_logging_level("INFO")

# load model
model = pybamm.lithium_ion.DFN()
model.convert_to_format = "python"

# create geometry
geometry = model.default_geometry

# load parameter values and process model and geometry
param = model.default_parameter_values
param.process_model(model)
param.process_geometry(geometry)

# set mesh
var = pybamm.standard_spatial_vars
var_pts = {var.x_n: 30, var.x_s: 30, var.x_p: 30, var.r_n: 10, var.r_p: 10}
mesh = pybamm.Mesh(geometry, model.default_submesh_types, var_pts)

# discretise model
disc = pybamm.Discretisation(mesh, model.default_spatial_methods)
disc.process_model(model)

# solve model
t_eval = np.linspace(0, 3600, 100)
solver = pybamm.IDAKLUSolver(root_method="lm")
solution = solver.solve(model, t_eval)

# plot
plot = pybamm.QuickPlot(
    solution,
    [
        "Negative particle concentration [mol.m-3]",
        "Electrolyte concentration [mol.m-3]",
        "Positive particle concentration [mol.m-3]",
        "Current [A]",
        "Negative electrode potential [V]",
        "Electrolyte potential [V]",
        "Positive electrode potential [V]",
        "Terminal voltage [V]",
    ],
    time_unit="seconds",
    spatial_unit="um",
)
plot.dynamic_plot()

[Saransh-cpp]

Oh, I'll run this on my side and check if I can resolve the bug.

[Saransh-cpp]

I was trying to run the above code and I faced an error ImportError: KLU is not installed, which I don't think is the bug you are mentioning. Then I explored the documentation again to look if I missed any requirements and I found that IDA based solvers are not available on windows right now. If that is the case then I won't be able to work on this issue because of the technical limitations. If there is something I missed and the error can be resolved using something different then please guide me. Thank you.

[valentinsulzer]

Yeah, the KLU solver isn't available on Windows. No worries

[valentinsulzer]

This bug is now fixed by #1449 . @Saransh-cpp do you still have the code to raise the error - if so can you open a PR?

[Saransh-cpp]

This bug is now fixed by #1449 . @Saransh-cpp do you still have the code to raise the error - if so can you open a PR?

Yes, I will create a PR with the code that was giving an error where it shouldn't have been. Where do you want me to add the code?

[valentinsulzer]

It should give a warning (not error, my bad) if model.convert_to_format is "casadi" in the KLU solver when the root_method is not CasadiAlgebraicSolver. The warning should indicate that the KLU solver is slower when using the casadi format

[Saransh-cpp]

Oh, I get it now, I will create a PR with the required warning.