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.

Sign up for GitHub

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

Disallow modifications to pybamm.Simulation object attributes after initialisation #3267

Merged
merged 13 commits into from
Sep 21, 2023
Merged

Disallow modifications to pybamm.Simulation object attributes after initialisation #3267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7f9c0e3
Remove setter for `parameter_values`, mark pararameter_values as private
agriyakhetarpal Aug 11, 2023
1f2e62b
Remove setter and mark `geometry` as private
agriyakhetarpal Aug 11, 2023
b93ed40
Remove setter for submesh types
agriyakhetarpal Aug 11, 2023
bc34631
Remove setter for spatial variable points
agriyakhetarpal Aug 11, 2023
8609cc4
Remove setters for spatial methods and output variables
agriyakhetarpal Aug 11, 2023
91ee8e1
Remove setter for solver options
agriyakhetarpal Aug 11, 2023
6dbf7d8
Remove setter and mark model inputs as private
agriyakhetarpal Aug 11, 2023
caeed50
Merge branch 'develop' into fix-simulation-api-behaviour
agriyakhetarpal Sep 5, 2023
9393f6f
Add CHANGELOG entry
agriyakhetarpal Sep 5, 2023
68584c1
Sort CHANGELOG
agriyakhetarpal Sep 6, 2023
82e00e3
Merge branch 'develop' into fix-simulation-api-behaviour
agriyakhetarpal Sep 19, 2023
91c8966
Update and sort CHANGELOG
agriyakhetarpal Sep 19, 2023
5cdf00b
Fix parameters semi-private object
agriyakhetarpal Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
- Fixed a bug that caused incorrect results of “{Domain} electrode thickness change [m]” due to the absence of dimension for the variable `electrode_thickness_change`([#3329](https://github.com/pybamm-team/PyBaMM/pull/3329)).
- Fixed a bug that occured in `check_ys_are_not_too_large` when trying to reference `y-slice` where the referenced variable was not a `pybamm.StateVector` ([#3313](https://github.com/pybamm-team/PyBaMM/pull/3313)
- Fixed a bug with `_Heaviside._evaluate_for_shape` which meant some expressions involving heaviside function and subtractions did not work ([#3306](https://github.com/pybamm-team/PyBaMM/pull/3306))
- Attributes of `pybamm.Simulation` objects (models, parameter values, geometries, choice of solver, and output variables) are now private and as such cannot be edited in-place after the simulation has been created ([#3267](https://github.com/pybamm-team/PyBaMM/pull/3267)
- Fixed bug causing incorrect activation energies using `create_from_bpx()` ([#3242](https://github.com/pybamm-team/PyBaMM/pull/3242))
- The `OneDimensionalX` thermal model has been updated to account for edge/tab cooling and account for the current collector volumetric heat capacity. It now gives the correct behaviour compared with a lumped model with the correct total heat transfer coefficient and surface area for cooling. ([#3042](https://github.com/pybamm-team/PyBaMM/pull/3042))
- Fixed a bug where the "basic" lithium-ion models gave incorrect results when using nonlinear particle diffusivity ([#3207](https://github.com/pybamm-team/PyBaMM/pull/3207))
- Particle size distributions now work with SPMe and NewmanTobias models ([#3207](https://github.com/pybamm-team/PyBaMM/pull/3207))
- Fix to simulate c_rate steps with drive cycles ([#3186](https://github.com/pybamm-team/PyBaMM/pull/3186))
Expand All @@ -24,6 +24,7 @@
- Thevenin() model is now constructed with standard variables: `Time [s]`, `Time [min]`, `Time [h]` ([#3143](https://github.com/pybamm-team/PyBaMM/pull/3143))
- Error generated when invalid parameter values are passed ([#3132](https://github.com/pybamm-team/PyBaMM/pull/3132))
- Parameters in `Prada2013` have been updated to better match those given in the paper, which is a 2.3 Ah cell, instead of the mix-and-match with the 1.1 Ah cell from Lain2019 ([#3096](https://github.com/pybamm-team/PyBaMM/pull/3096))
- The `OneDimensionalX` thermal model has been updated to account for edge/tab cooling and account for the current collector volumetric heat capacity. It now gives the correct behaviour compared with a lumped model with the correct total heat transfer coefficient and surface area for cooling. ([#3042](https://github.com/pybamm-team/PyBaMM/pull/3042))

## Optimizations

Expand Down
99 changes: 33 additions & 66 deletions pybamm/simulation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import pickle
import pybamm
import numpy as np
import copy
import warnings
import sys
from functools import lru_cache
Expand Down Expand Up @@ -74,8 +73,8 @@ def __init__(
output_variables=None,
C_rate=None,
):
self.parameter_values = parameter_values or model.default_parameter_values
self._unprocessed_parameter_values = self.parameter_values
self._parameter_values = parameter_values or model.default_parameter_values
self._unprocessed_parameter_values = self._parameter_values

if isinstance(model, pybamm.lithium_ion.BasicDFNHalfCell):
if experiment is not None:
Expand Down Expand Up @@ -114,14 +113,14 @@ def __init__(
self.experiment = experiment.copy()

self._unprocessed_model = model
self.model = model
self._model = model

self.geometry = geometry or self.model.default_geometry
self.submesh_types = submesh_types or self.model.default_submesh_types
self.var_pts = var_pts or self.model.default_var_pts
self.spatial_methods = spatial_methods or self.model.default_spatial_methods
self.solver = solver or self.model.default_solver
self.output_variables = output_variables
self._geometry = geometry or self._model.default_geometry
self._submesh_types = submesh_types or self._model.default_submesh_types
self._var_pts = var_pts or self._model.default_var_pts
self._spatial_methods = spatial_methods or self._model.default_spatial_methods
self._solver = solver or self._model.default_solver
self._output_variables = output_variables

# Initialize empty built states
self._model_with_set_params = None
Expand Down Expand Up @@ -201,16 +200,16 @@ def set_up_and_parameterise_experiment(self):

def set_up_and_parameterise_model_for_experiment(self):
"""
Set up self.model to be able to run the experiment (new version).
Set up self._model to be able to run the experiment (new version).
In this version, a new model is created for each step.

This increases set-up time since several models to be processed, but
reduces simulation time since the model formulation is efficient.
"""
self.experiment_unique_steps_to_model = {}
for op_number, op in enumerate(self.experiment.unique_steps):
new_model = self.model.new_copy()
new_parameter_values = self.parameter_values.copy()
new_model = self._model.new_copy()
new_parameter_values = self._parameter_values.copy()

if op.type != "current":
# Voltage or power control
Expand Down Expand Up @@ -259,9 +258,9 @@ def set_up_and_parameterise_model_for_experiment(self):

# Set up rest model if experiment has start times
if self.experiment.initial_start_time:
new_model = self.model.new_copy()
new_model = self._model.new_copy()
# Update parameter values
new_parameter_values = self.parameter_values.copy()
new_parameter_values = self._parameter_values.copy()
self._original_temperature = new_parameter_values["Ambient temperature [K]"]
new_parameter_values.update(
{"Current function [A]": 0, "Ambient temperature [K]": "[input]"},
Expand Down Expand Up @@ -360,8 +359,8 @@ def set_parameters(self):
self._model_with_set_params = self._parameter_values.process_model(
self._unprocessed_model, inplace=False
)
self._parameter_values.process_geometry(self.geometry)
self.model = self._model_with_set_params
self._parameter_values.process_geometry(self._geometry)
self._model = self._model_with_set_params

def set_initial_soc(self, initial_soc):
if self._built_initial_soc != initial_soc:
Expand All @@ -372,13 +371,13 @@ def set_initial_soc(self, initial_soc):
self.op_conds_to_built_solvers = None

options = self.model.options
param = self.model.param
param = self._model.param
if options["open-circuit potential"] == "MSMR":
self.parameter_values = self._unprocessed_parameter_values.set_initial_ocps(
self._parameter_values = self._unprocessed_parameter_values.set_initial_ocps( # noqa: E501
initial_soc, param=param, inplace=False, options=options
)
else:
self.parameter_values = (
self._parameter_values = (
self._unprocessed_parameter_values.set_initial_stoichiometries(
initial_soc, param=param, inplace=False, options=options
)
Expand Down Expand Up @@ -410,9 +409,9 @@ def build(self, check_model=True, initial_soc=None):

if self.built_model:
return
elif self.model.is_discretised:
self._model_with_set_params = self.model
self._built_model = self.model
elif self._model.is_discretised:
self._model_with_set_params = self._model
self._built_model = self._model
else:
self.set_parameters()
self._mesh = pybamm.Mesh(self._geometry, self._submesh_types, self._var_pts)
Expand Down Expand Up @@ -454,7 +453,7 @@ def build_for_experiment(self, check_model=True, initial_soc=None):
built_model = self._disc.process_model(
model_with_set_params, inplace=True, check_model=check_model
)
solver = self.solver.copy()
solver = self._solver.copy()
self.op_conds_to_built_solvers[op_cond] = solver
self.op_conds_to_built_models[op_cond] = built_model

Expand Down Expand Up @@ -526,7 +525,7 @@ def solve(
"""
# Setup
if solver is None:
solver = self.solver
solver = self._solver

callbacks = pybamm.callbacks.setup_callbacks(callbacks)
logs = {}
Expand All @@ -544,7 +543,7 @@ def solve(
)
if (
self.operating_mode == "without experiment"
or self.model.name == "ElectrodeSOH model"
or self._model.name == "ElectrodeSOH model"
):
if t_eval is None:
raise pybamm.SolverError(
Expand Down Expand Up @@ -908,7 +907,7 @@ def solve(
# Calculate capacity_start using the first cycle
if cycle_num == 1:
# Note capacity_start could be defined as
# self.parameter_values["Nominal cell capacity [A.h]"] instead
# self._parameter_values["Nominal cell capacity [A.h]"] instead
if "capacity" in self.experiment.termination:
capacity_start = all_summary_variables[0]["Capacity [A.h]"]
logs["start capacity"] = capacity_start
Expand Down Expand Up @@ -1000,7 +999,7 @@ def step(
self.build()

if solver is None:
solver = self.solver
solver = self._solver

if starting_solution is None:
starting_solution = self._solution
Expand All @@ -1014,14 +1013,14 @@ def step(
def _get_esoh_solver(self, calc_esoh):
if (
calc_esoh is False
or isinstance(self.model, pybamm.lead_acid.BaseModel)
or isinstance(self.model, pybamm.equivalent_circuit.Thevenin)
or self.model.options["working electrode"] != "both"
or isinstance(self._model, pybamm.lead_acid.BaseModel)
or isinstance(self._model, pybamm.equivalent_circuit.Thevenin)
or self._model.options["working electrode"] != "both"
):
return None

return pybamm.lithium_ion.ElectrodeSOHSolver(
self.parameter_values, self.model.param, options=self.model.options
self._parameter_values, self._model.param, options=self._model.options
)

def plot(self, output_variables=None, **kwargs):
Expand All @@ -1046,7 +1045,7 @@ def plot(self, output_variables=None, **kwargs):
)

if output_variables is None:
output_variables = self.output_variables
output_variables = self._output_variables

self.quick_plot = pybamm.dynamic_plot(
self._solution, output_variables=output_variables, **kwargs
Expand Down Expand Up @@ -1082,10 +1081,6 @@ def create_gif(self, number_of_images=80, duration=0.1, output_filename="plot.gi
def model(self):
return self._model

@model.setter
def model(self, model):
self._model = copy.copy(model)

@property
def model_with_set_params(self):
return self._model_with_set_params
Expand All @@ -1098,26 +1093,14 @@ def built_model(self):
def geometry(self):
return self._geometry

@geometry.setter
def geometry(self, geometry):
self._geometry = geometry

@property
def parameter_values(self):
return self._parameter_values

@parameter_values.setter
def parameter_values(self, parameter_values):
self._parameter_values = parameter_values.copy()

@property
def submesh_types(self):
return self._submesh_types

@submesh_types.setter
def submesh_types(self, submesh_types):
self._submesh_types = submesh_types.copy()

@property
def mesh(self):
return self._mesh
Expand All @@ -1126,41 +1109,25 @@ def mesh(self):
def var_pts(self):
return self._var_pts

@var_pts.setter
def var_pts(self, var_pts):
self._var_pts = var_pts.copy()

@property
def spatial_methods(self):
return self._spatial_methods

@spatial_methods.setter
def spatial_methods(self, spatial_methods):
self._spatial_methods = spatial_methods.copy()

@property
def solver(self):
return self._solver

@solver.setter
def solver(self, solver):
self._solver = solver.copy()

@property
def output_variables(self):
return self._output_variables

@output_variables.setter
def output_variables(self, output_variables):
self._output_variables = copy.copy(output_variables)

@property
def solution(self):
return self._solution

def save(self, filename):
"""Save simulation using pickle"""
if self.model.convert_to_format == "python":
if self._model.convert_to_format == "python":
# We currently cannot save models in the 'python' format
raise NotImplementedError(
"""
Expand Down
Loading