Update NCA_Kim2011.py

[1836005678]

Fixing #2744 to make sure we hit the voltage cut-off, instead of the particle stoichiometry going above 1.
Using the function in Kim2011 article to take the place of pybamm\input\parameters\lithium_ion\data\nca_ocp_Kim2011_data.csv for parameter 'Positive electrode OCP[V]' in Kim2011 parameter set.
The function 'nca_ocp_Kim2011(sto)' in pybamm\input\parameters\lithium_ion\NCA_Kim2011.py is edited.

[valentinsulzer]

Looks good, you just need to remove import os in this file since it is no longer used

[valentinsulzer]

Hmm, tests are failing because the old NCA Kim error was being used to test for the error reporting, if you feel confident to fix it I would suggest adding a different example where the interpolant extrapolates instead. I think something like this should work

x = np.linspace(0,2)
var = pybamm.Variable("var")
rhs = pybamm.Interpolant(x, x, var, name=name, interpolator="linear")
model = pybamm.BaseModel()
model.rhs[var] = rhs
model.initial_conditions[var] = 1
solver = pybamm.ScipySolver()
t_eval = [0,5]

# solution should grow exponentially and go above var=2, then the interpolant should error
solver.solve(model, t_eval)

But you should test this locally first to make sure it does indeed raise the error. Let me know if you need help

[1836005678]

Sorry, I don't know what 'old NCA Kim error was being used to test for the error reporting' means. Could you explain it in detail?

[valentinsulzer]

This test is failing since line 508 expects an error from the parameter set before you fixed it.

PyBaMM/tests/unit/test_solvers/test_casadi_solver.py

Lines 488 to 509 in f45f4a5

	def test_interpolant_extrapolate(self):
	model = pybamm.lithium_ion.DFN()
	param = pybamm.ParameterValues("NCA_Kim2011")
	experiment = pybamm.Experiment(
	["Charge at 1C until 4.2 V"], period="10 seconds"
	)
	param["Upper voltage cut-off [V]"] = 4.8
	sim = pybamm.Simulation(
	model,
	parameter_values=param,
	experiment=experiment,
	solver=pybamm.CasadiSolver(
	mode="safe",
	dt_max=0.001,
	extrap_tol=1e-3,
	extra_options_setup={"max_num_steps": 500},
	),
	)
	with self.assertRaisesRegex(pybamm.SolverError, "interpolation bounds"):
	sim.solve()

Instead, we should test using a standalone piece of code like the one I shared above, and still make sure the error is raised

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (7ff45ec) 99.69% compared to head (d773b8d) 99.68%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2751      +/-   ##
===========================================
- Coverage    99.69%   99.68%   -0.01%     
===========================================
  Files          272      272              
  Lines        19034    19031       -3     
===========================================
- Hits         18975    18972       -3     
  Misses          59       59              

Impacted Files	Coverage Δ
pybamm/input/parameters/lithium_ion/NCA_Kim2011.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[1836005678]

Although I still don't quite understand the reason and purpose of this test, I am very grateful for this update and your work.