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

Allow drive cycles in experiments #1193

Closed
valentinsulzer opened this issue Oct 17, 2020 · 20 comments
Closed

Allow drive cycles in experiments #1193

valentinsulzer opened this issue Oct 17, 2020 · 20 comments

Comments

@valentinsulzer
Copy link
Member

valentinsulzer commented Oct 17, 2020

Add functionality to allow running a drive cycle as part of an experiment. For example:

pybamm.Experiment(["Run US06 for 1 hour"], functions={"US06": US06_function})

Currently, the way experiments are implemented require current/voltage/power to be constant (they are set up as InputParameter objects). So this could be challenging to implement, and would depend on #895 being possible.

@maurosgroi
Copy link

Dear @tinosulzer,
I'm trying to use this functionality to import an extended WLTP cycle for HD vehicles lasting more than 2800s. The sampling time is 0.1s.
Essentially I'm substuting the original csv file with mine but the solution is cut at about 770 s.
Is there any limitation in the number of point that can be imported?
My csv file for reference is uploaded here: https://drive.google.com/file/d/1MJBdq-g5RjednvSJ8e-LMkQnnxe1OWvj/view?usp=sharing

Thanks a lot in advance and best regards,
Mauro Sgroi.

@valentinsulzer
Copy link
Member Author

This functionality is not implemented yet as described above. Can you show me the code you are using to load your drive cycle?

@maurosgroi
Copy link

Here it is. I tried to reuse part of tutorial4.

import pybamm
import pandas as pd # needed to read the csv data file
chemistry = pybamm.parameter_sets.Chen2020
parameter_values = pybamm.ParameterValues(chemistry=chemistry)
model = pybamm.lithium_ion.SPM()
drive_cycle = pd.read_csv("D:/WLTC.csv", comment="#", header=None).to_numpy()
timescale = parameter_values.evaluate(model.timescale)
current_interpolant = pybamm.Interpolant(drive_cycle, timescale * pybamm.t)

parameter_values["Current function [A]"] = current_interpolant
sim = pybamm.Simulation(model, parameter_values=parameter_values)
sim.solve(t_eval=None)
sim.plot(["Current [A]", "Terminal voltage [V]"], time_unit='seconds', variable_limits='tight')

Thanks a lot and best regards,
Mauro Sgroi.

@maurosgroi
Copy link

Sorry, I misunderstood your initial post.
Anyway I cannot my cycle work.
Thanks a lot and best regards,
Mauro Sgroi.

@valentinsulzer
Copy link
Member Author

Can you describe what you are trying to do? Would be good to know, in case it is easy to implement

@maurosgroi
Copy link

The final goal is to simulate an experiment composed by a multistep charge followed by a series of WLTP cycles.
But for the moment I cannot run correctly the simulation with just the WLTC alone.

@maurosgroi
Copy link

Dear @tinosulzer,
is it better to open a specific topic for my problem?
Mayebe here it is not appropriate?
Best regards,
Mauro.

@valentinsulzer
Copy link
Member Author

Yes, please open a separate issue and I or someone else will look into it

@brosaplanella
Copy link
Member

To add to this issue, it would be good to generalise it so you can pass drive cycles for current, voltage or power. Not sure if they all have the same level of complexity or not.

@valentinsulzer
Copy link
Member Author

Yes, good idea, that would be under the same level of complexity

@valentinsulzer
Copy link
Member Author

@Saransh-cpp could you try this? Basically it now just requires passing the drive cycle interpolant to

PyBaMM/pybamm/simulation.py

Lines 437 to 439 in 40a114e

new_parameter_values.update(
{"Current function [A]": op_inputs["Current input [A]"]}
)
instead of a scalar current input. In that case "Current input [A]" should be removed from op_inputs as it will cause problems later on if it's not a scalar.

@Saransh-cpp
Copy link
Member

@Saransh-cpp could you try this? Basically it now just requires passing the drive cycle interpolant to

PyBaMM/pybamm/simulation.py

Lines 437 to 439 in 40a114e

new_parameter_values.update(
{"Current function [A]": op_inputs["Current input [A]"]}
)

instead of a scalar current input. In that case "Current input [A]" should be removed from op_inputs as it will cause problems later on if it's not a scalar.

Could you please elaborate more on this? Should I remove "Current input [A]" completely from op_inputs (and operating_inputs?) as we are passing the drive cycle interpolant to it now (and it will be probably present in some tests too, so I will check all the files once if needed)?

@valentinsulzer
Copy link
Member Author

With the new setup type the Current input (and actually voltage input and power input) are only needed for the parameter values. So once you update the parameter values you can throw them away. Something like

I = op_inputs.pop("Current input [A]")
new_parameter_values.update("Current function [A]": I)

might just work out of the box. Since op_inputs is a pointer to an object in self._experiment_inputs this will also remove "Current input [A]" from the relevant dictionary in self._experiment_inputs. operating_inputs should be unchanged

@Saransh-cpp
Copy link
Member

Saransh-cpp commented Mar 30, 2021

With the new setup type the Current input (and actually voltage input and power input) are only needed for the parameter values. So once you update the parameter values you can throw them away. Something like

I = op_inputs.pop("Current input [A]")
new_parameter_values.update("Current function [A]": I)

might just work out of the box. Since op_inputs is a pointer to an object in self._experiment_inputs this will also remove "Current input [A]" from the relevant dictionary in self._experiment_inputs. operating_inputs should be unchanged

I will definitely look into this and if everything goes fine, I will make a PR.

@Saransh-cpp
Copy link
Member

@tinosulzer could you please confirm the format in which the new drive cycle experiments can be simulated? The code below gives an error (which I think it should because my experiment doesn't contain "A", "C", "V" or "W") -

line 214, in set_up_experiment
    operating_inputs["period"] = op[3]
UnboundLocalError: local variable 'operating_inputs' referenced before assignment

Code -

import pybamm
import pandas as pd
import numpy as np

model = pybamm.lithium_ion.DFN()
params = model.default_parameter_values

drive_cycle = pd.read_csv("US06.csv", comment="#", header=None).to_numpy()

experiment = pybamm.Experiment(["Run US06 for 1 hour"], drive_cycles={"US06": drive_cycle}, period="20 seconds")

sim = pybamm.Simulation(model, experiment=experiment, solver=pybamm.CasadiSolver(mode="fast"))

sim.solve()
solution = sim.solution

pybamm.dynamic_plot(solution)

I also ran basic simulations by changing "Current function [A]" using Interpolant or by giving it a constant value, after adding

I = op_inputs.pop("Current input [A]")
new_parameter_values.update("Current function [A]": I)

to simulation.py and everything worked fine.

@valentinsulzer
Copy link
Member Author

Ah yeah. Looking at it again I think there should be a specifier in the experiment class for what kind of data the drive cycle is giving, e.g.

"Run US06 (A) for 1 hour"

and then in the Experiment class this code

electric = (ext_drive_cycle[:, 1], "Drive")
time = ext_drive_cycle[:, 0][-1]

should be replaced with

interp = ... # define the interpolant
typ = ... # find the type ("A", "V", or "W")
electric = (interp, typ)

Maybe @alibh95 can help with that.
Then you can read the interpolant directly in the Simulation class

Note that the simulation calls different functions depending on whether or not you supply an experiment (it doesn't call the code you changed if you don't supply an experiment)

@Saransh-cpp
Copy link
Member

Oh yes, running experiments with the edited code does give an error -

np.all(abs(model.casadi_algebraic(t, y0, inputs).full()) < self.tol)
as mismatching shape. Got 9-by-1. Allowed dimensions, in general, are:
 - The input dimension N-by-M (here 8-by-1)
 - A scalar, i.e. 1-by-1
 - M-by-N if N=1 or M=1 (i.e. a transposed vector)
 - N-by-M1 if K*M1=M for some K (argument repeated horizontally)
 - N-by-P*M, indicating evaluation with multiple arguments (P must be a multiple of 1 for consistency with previous inputs)

I will try to work around this (exploring the code more for now) and see if I can find something.

@Saransh-cpp
Copy link
Member

With the new setup type the Current input (and actually voltage input and power input) are only needed for the parameter values. So once you update the parameter values you can throw them away. Something like

I = op_inputs.pop("Current input [A]")
new_parameter_values.update("Current function [A]": I)

might just work out of the box. Since op_inputs is a pointer to an object in self._experiment_inputs this will also remove "Current input [A]" from the relevant dictionary in self._experiment_inputs. operating_inputs should be unchanged

Popping out "Current input [A]" does work and removes it from both op_inputs and self._experiment_inputs but gives an error -

    return _casadi.Function_call(self, *args)
RuntimeError: .../casadi/core/function_internal.hpp:1257: Input 2 (i2) has mismatching shape. 
Got 9-by-1. Allowed dimensions, in general, are:
 - The input dimension N-by-M (here 8-by-1)
 - A scalar, i.e. 1-by-1
 - M-by-N if N=1 or M=1 (i.e. a transposed vector)
 - N-by-M1 if K*M1=M for some K (argument repeated horizontally)
 - N-by-P*M, indicating evaluation with multiple arguments (P must be a multiple of 1 for consistency with previous inputs)

on solving a simulated experiment. I have been trying to solve this but I haven't discovered a solution yet. Making the "input" values 0 does work but again 0 is a scalar so that is not something desirable.

@DavidMStraub
Copy link
Contributor

Hi, I've seen that @alibh95 contributed features related to this issue in #1304 and #1457, so I've been wondering, is this issue resolved or is there still something missing?

@valentinsulzer
Copy link
Member Author

Closed via #1793

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants