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

Issue 933 remove reactions dict #948

Merged
merged 23 commits into from
Apr 29, 2020
Merged

Issue 933 remove reactions dict #948

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
66dd222
#933 start removing reactions dict
valentinsulzer Apr 6, 2020
2181a54
Merge branch 'develop' into issue-933-remove-reactions-dict
valentinsulzer Apr 6, 2020
5957a80
#933 remove reactions dict
valentinsulzer Apr 6, 2020
18e7fd4
#933 fix tests
valentinsulzer Apr 6, 2020
d6a7f39
#933 merge develop
valentinsulzer Apr 8, 2020
23c6e2a
#933 merge #546
valentinsulzer Apr 9, 2020
4fdf3d8
#933 fix tests except public functions
valentinsulzer Apr 9, 2020
b3b78fd
Merge branch 'develop' of github.com:pybamm-team/PyBaMM into issue-93…
valentinsulzer Apr 9, 2020
3fc821f
Merge branch 'develop' into issue-933-remove-reactions-dict
valentinsulzer Apr 13, 2020
ef04516
Merge branch 'develop' of github.com:pybamm-team/PyBaMM into issue-93…
valentinsulzer Apr 14, 2020
e0fce7d
#933 debugging
valentinsulzer Apr 14, 2020
82a5cdf
Merge branch 'develop' into issue-933-remove-reactions-dict
valentinsulzer Apr 19, 2020
0450158
Merge branch 'develop' into issue-933-remove-reactions-dict
valentinsulzer Apr 24, 2020
0aea591
#933 fix unit tests
valentinsulzer Apr 24, 2020
a338812
#933 fixing tests
valentinsulzer Apr 24, 2020
9af70dd
#933 fix integration tests
valentinsulzer Apr 24, 2020
68bdfd9
#933 fix weirdly broken test ...
valentinsulzer Apr 24, 2020
0eb8e14
#933 merge
valentinsulzer Apr 29, 2020
43f9243
Merge branch 'issue-963-misc-bugs' into issue-933-remove-reactions-dict
valentinsulzer Apr 29, 2020
8d1daa8
#933 fix test
valentinsulzer Apr 29, 2020
23522eb
Merge branch 'issue-963-misc-bugs' into issue-933-remove-reactions-dict
valentinsulzer Apr 29, 2020
c270b12
#933 fix example
valentinsulzer Apr 29, 2020
6473727
Merge branch 'develop' into issue-933-remove-reactions-dict
valentinsulzer Apr 29, 2020
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
2 changes: 1 addition & 1 deletion examples/scripts/compare_lead_acid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

# load parameter values and process models and geometry
param = models[0].default_parameter_values
param.update({"Current function [A]": 85, "Initial State of Charge": 1})
param.update({"Current function [A]": 17, "Initial State of Charge": 1})
for model in models:
param.process_model(model)

Expand Down
3 changes: 1 addition & 2 deletions pybamm/models/full_battery_models/base_battery_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,7 @@ def options(self, extra_options):
):
if len(options["side reactions"]) > 0:
raise pybamm.OptionError(
"""
must use surface formulation to solve {!s} with side reactions
"""must use surface formulation to solve {!s} with side reactions
""".format(
self
)
Expand Down
28 changes: 0 additions & 28 deletions pybamm/models/full_battery_models/lead_acid/base_lead_acid_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,34 +53,6 @@ def default_solver(self):
else:
return pybamm.CasadiSolver(mode="safe")

def set_reactions(self):

# Should probably refactor as this is a bit clunky at the moment
# Maybe each reaction as a Reaction class so we can just list names of classes
param = self.param
icd = " interfacial current density"
self.reactions = {
"main": {
"Negative": {"s": -param.s_plus_n_S, "aj": "Negative electrode" + icd},
"Positive": {"s": -param.s_plus_p_S, "aj": "Positive electrode" + icd},
}
}
if "oxygen" in self.options["side reactions"]:
self.reactions["oxygen"] = {
"Negative": {
"s": -param.s_plus_Ox,
"s_ox": -param.s_ox_Ox,
"aj": "Negative electrode oxygen" + icd,
},
"Positive": {
"s": -param.s_plus_Ox,
"s_ox": -param.s_ox_Ox,
"aj": "Positive electrode oxygen" + icd,
},
}
self.reactions["main"]["Negative"]["s_ox"] = 0
self.reactions["main"]["Positive"]["s_ox"] = 0

def set_soc_variables(self):
"Set variables relating to the state of charge."
# State of Charge defined as function of dimensionless electrolyte concentration
Expand Down
15 changes: 7 additions & 8 deletions pybamm/models/full_battery_models/lead_acid/full.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ def __init__(self, options=None, name="Full model", build=True):
super().__init__(options, name)

self.set_external_circuit_submodel()
self.set_reactions()
self.set_interfacial_submodel()
self.set_porosity_submodel()
self.set_tortuosity_submodels()
Expand Down Expand Up @@ -85,8 +84,8 @@ def set_interfacial_submodel(self):

def set_solid_submodel(self):
if self.options["surface form"] is False:
submod_n = pybamm.electrode.ohm.Full(self.param, "Negative", self.reactions)
submod_p = pybamm.electrode.ohm.Full(self.param, "Positive", self.reactions)
submod_n = pybamm.electrode.ohm.Full(self.param, "Negative")
submod_p = pybamm.electrode.ohm.Full(self.param, "Positive")
else:
submod_n = pybamm.electrode.ohm.SurfaceForm(self.param, "Negative")
submod_p = pybamm.electrode.ohm.SurfaceForm(self.param, "Positive")
Expand All @@ -99,28 +98,28 @@ def set_electrolyte_submodel(self):
surf_form = pybamm.electrolyte_conductivity.surface_potential_form

self.submodels["electrolyte diffusion"] = pybamm.electrolyte_diffusion.Full(
self.param, self.reactions
self.param
)

if self.options["surface form"] is False:
self.submodels[
"electrolyte conductivity"
] = pybamm.electrolyte_conductivity.Full(self.param, self.reactions)
] = pybamm.electrolyte_conductivity.Full(self.param)
elif self.options["surface form"] == "differential":
for domain in ["Negative", "Separator", "Positive"]:
self.submodels[
domain.lower() + " electrolyte conductivity"
] = surf_form.FullDifferential(self.param, domain, self.reactions)
] = surf_form.FullDifferential(self.param, domain)
elif self.options["surface form"] == "algebraic":
for domain in ["Negative", "Separator", "Positive"]:
self.submodels[
domain.lower() + " electrolyte conductivity"
] = surf_form.FullAlgebraic(self.param, domain, self.reactions)
] = surf_form.FullAlgebraic(self.param, domain)

def set_side_reaction_submodels(self):
if "oxygen" in self.options["side reactions"]:
self.submodels["oxygen diffusion"] = pybamm.oxygen_diffusion.Full(
self.param, self.reactions
self.param
)
self.submodels["positive oxygen interface"] = pybamm.interface.ForwardTafel(
self.param, "Positive", "lead-acid oxygen"
Expand Down
19 changes: 8 additions & 11 deletions pybamm/models/full_battery_models/lead_acid/higher_order.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ def __init__(self, options=None, name="Composite model", build=True):

self.set_external_circuit_submodel()
self.set_leading_order_model()
self.set_reactions()
# Electrolyte submodel to get first-order concentrations
self.set_electrolyte_diffusion_submodel()
self.set_other_species_diffusion_submodels()
Expand Down Expand Up @@ -205,12 +204,12 @@ def __init__(self, options=None, name="FOQS model", build=True):
def set_electrolyte_diffusion_submodel(self):
self.submodels[
"electrolyte diffusion"
] = pybamm.electrolyte_diffusion.FirstOrder(self.param, self.reactions)
] = pybamm.electrolyte_diffusion.FirstOrder(self.param)

def set_other_species_diffusion_submodels(self):
if "oxygen" in self.options["side reactions"]:
self.submodels["oxygen diffusion"] = pybamm.oxygen_diffusion.FirstOrder(
self.param, self.reactions
self.param
)

def set_full_porosity_submodel(self):
Expand All @@ -235,12 +234,12 @@ def __init__(self, options=None, name="Composite model", build=True):
def set_electrolyte_diffusion_submodel(self):
self.submodels[
"electrolyte diffusion"
] = pybamm.electrolyte_diffusion.Composite(self.param, self.reactions)
] = pybamm.electrolyte_diffusion.Composite(self.param)

def set_other_species_diffusion_submodels(self):
if "oxygen" in self.options["side reactions"]:
self.submodels["oxygen diffusion"] = pybamm.oxygen_diffusion.Composite(
self.param, self.reactions
self.param
)

def set_full_porosity_submodel(self):
Expand Down Expand Up @@ -279,14 +278,12 @@ def __init__(
def set_electrolyte_diffusion_submodel(self):
self.submodels[
"electrolyte diffusion"
] = pybamm.electrolyte_diffusion.Composite(
self.param, self.reactions, extended="distributed"
)
] = pybamm.electrolyte_diffusion.Composite(self.param, extended="distributed")

def set_other_species_diffusion_submodels(self):
if "oxygen" in self.options["side reactions"]:
self.submodels["oxygen diffusion"] = pybamm.oxygen_diffusion.Composite(
self.param, self.reactions, extended="distributed"
self.param, extended="distributed"
)


Expand All @@ -304,11 +301,11 @@ def set_electrolyte_diffusion_submodel(self):
self.submodels[
"electrolyte diffusion"
] = pybamm.electrolyte.stefan_maxwell.diffusion.Composite(
self.param, self.reactions, extended="average"
self.param, extended="average"
)

def set_other_species_diffusion_submodels(self):
if "oxygen" in self.options["side reactions"]:
self.submodels["oxygen diffusion"] = pybamm.oxygen_diffusion.Composite(
self.param, self.reactions, extended="average"
self.param, extended="average"
)
11 changes: 4 additions & 7 deletions pybamm/models/full_battery_models/lead_acid/loqs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ def __init__(self, options=None, name="LOQS model", build=True):
super().__init__(options, name)

self.set_external_circuit_submodel()
self.set_reactions()
self.set_interfacial_submodel()
self.set_convection_submodel()
self.set_porosity_submodel()
Expand Down Expand Up @@ -186,25 +185,23 @@ def set_electrolyte_submodel(self):
for domain in ["Negative", "Separator", "Positive"]:
self.submodels[
"leading-order " + domain.lower() + " electrolyte conductivity"
] = surf_form.LeadingOrderDifferential(
self.param, domain, self.reactions
)
] = surf_form.LeadingOrderDifferential(self.param, domain)

elif self.options["surface form"] == "algebraic":
for domain in ["Negative", "Separator", "Positive"]:
self.submodels[
"leading-order " + domain.lower() + " electrolyte conductivity"
] = surf_form.LeadingOrderAlgebraic(self.param, domain, self.reactions)
] = surf_form.LeadingOrderAlgebraic(self.param, domain)

self.submodels[
"electrolyte diffusion"
] = pybamm.electrolyte_diffusion.LeadingOrder(self.param, self.reactions)
] = pybamm.electrolyte_diffusion.LeadingOrder(self.param)

def set_side_reaction_submodels(self):
if "oxygen" in self.options["side reactions"]:
self.submodels[
"leading-order oxygen diffusion"
] = pybamm.oxygen_diffusion.LeadingOrder(self.param, self.reactions)
] = pybamm.oxygen_diffusion.LeadingOrder(self.param)
self.submodels[
"leading-order positive oxygen interface"
] = pybamm.interface.ForwardTafel(
Expand Down
18 changes: 7 additions & 11 deletions pybamm/models/full_battery_models/lithium_ion/base_lithium_ion_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,10 @@ def set_standard_output_variables(self):
}
)

def set_reactions(self):

# Should probably refactor as this is a bit clunky at the moment
# Maybe each reaction as a Reaction class so we can just list names of classes
icd = " interfacial current density"
self.reactions = {
"main": {
"Negative": {"s": 1, "aj": "Negative electrode" + icd},
"Positive": {"s": 1, "aj": "Positive electrode" + icd},
}
}
def set_other_reaction_submodels_to_zero(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of having set_other_reaction_submodels_to_zero could we have something like set_reaction_submodels(self,[list of reactions]): which then sets the appropriate models from the list (and sets NoReaction for things that aren't in the list)? I'm just thinking of how this extends in the future and works with model options. It would be nice to have an option "reactions" which takes a list of names of any reactions you want to include. Actually it looks like this already happens, and these are always zero for li-ion. Maybe there could be a comment or this method renamed, as seeing it in e.g. DFN was a little confusing -- it wasn't clear which reactions this referred to.

self.submodels["negative oxygen interface"] = pybamm.interface.NoReaction(
self.param, "Negative", "lithium-ion oxygen"
)
self.submodels["positive oxygen interface"] = pybamm.interface.NoReaction(
self.param, "Positive", "lithium-ion oxygen"
)
14 changes: 7 additions & 7 deletions pybamm/models/full_battery_models/lithium_ion/dfn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ def __init__(self, options=None, name="Doyle-Fuller-Newman model", build=True):
super().__init__(options, name)

self.set_external_circuit_submodel()
self.set_reactions()
self.set_porosity_submodel()
self.set_tortuosity_submodels()
self.set_convection_submodel()
self.set_interfacial_submodel()
self.set_other_reaction_submodels_to_zero()
self.set_particle_submodel()
self.set_solid_submodel()
self.set_electrolyte_submodel()
Expand Down Expand Up @@ -92,8 +92,8 @@ def set_particle_submodel(self):
def set_solid_submodel(self):

if self.options["surface form"] is False:
submod_n = pybamm.electrode.ohm.Full(self.param, "Negative", self.reactions)
submod_p = pybamm.electrode.ohm.Full(self.param, "Positive", self.reactions)
submod_n = pybamm.electrode.ohm.Full(self.param, "Negative")
submod_p = pybamm.electrode.ohm.Full(self.param, "Positive")
else:
submod_n = pybamm.electrode.ohm.SurfaceForm(self.param, "Negative")
submod_p = pybamm.electrode.ohm.SurfaceForm(self.param, "Positive")
Expand All @@ -106,23 +106,23 @@ def set_electrolyte_submodel(self):
surf_form = pybamm.electrolyte_conductivity.surface_potential_form

self.submodels["electrolyte diffusion"] = pybamm.electrolyte_diffusion.Full(
self.param, self.reactions
self.param
)

if self.options["surface form"] is False:
self.submodels[
"electrolyte conductivity"
] = pybamm.electrolyte_conductivity.Full(self.param, self.reactions)
] = pybamm.electrolyte_conductivity.Full(self.param)
elif self.options["surface form"] == "differential":
for domain in ["Negative", "Separator", "Positive"]:
self.submodels[
domain.lower() + " electrolyte conductivity"
] = surf_form.FullDifferential(self.param, domain, self.reactions)
] = surf_form.FullDifferential(self.param, domain)
elif self.options["surface form"] == "algebraic":
for domain in ["Negative", "Separator", "Positive"]:
self.submodels[
domain.lower() + " electrolyte conductivity"
] = surf_form.FullAlgebraic(self.param, domain, self.reactions)
] = surf_form.FullAlgebraic(self.param, domain)

@property
def default_geometry(self):
Expand Down
8 changes: 3 additions & 5 deletions pybamm/models/full_battery_models/lithium_ion/spm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ class SPM(BaseModel):
def __init__(self, options=None, name="Single Particle Model", build=True):
super().__init__(options, name)

self.set_reactions()
self.set_external_circuit_submodel()
self.set_porosity_submodel()
self.set_tortuosity_submodels()
self.set_convection_submodel()
self.set_interfacial_submodel()
self.set_other_reaction_submodels_to_zero()
self.set_particle_submodel()
self.set_negative_electrode_submodel()
self.set_electrolyte_submodel()
Expand Down Expand Up @@ -123,15 +123,13 @@ def set_electrolyte_submodel(self):
for domain in ["Negative", "Separator", "Positive"]:
self.submodels[
"leading-order " + domain.lower() + " electrolyte conductivity"
] = surf_form.LeadingOrderDifferential(
self.param, domain, self.reactions
)
] = surf_form.LeadingOrderDifferential(self.param, domain)

elif self.options["surface form"] == "algebraic":
for domain in ["Negative", "Separator", "Positive"]:
self.submodels[
"leading-order " + domain.lower() + " electrolyte conductivity"
] = surf_form.LeadingOrderAlgebraic(self.param, domain, self.reactions)
] = surf_form.LeadingOrderAlgebraic(self.param, domain)
self.submodels[
"electrolyte diffusion"
] = pybamm.electrolyte_diffusion.ConstantConcentration(self.param)
Expand Down
4 changes: 2 additions & 2 deletions pybamm/models/full_battery_models/lithium_ion/spme.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ def __init__(
super().__init__(options, name)

self.set_external_circuit_submodel()
self.set_reactions()
self.set_porosity_submodel()
self.set_tortuosity_submodels()
self.set_convection_submodel()
self.set_interfacial_submodel()
self.set_other_reaction_submodels_to_zero()
self.set_particle_submodel()
self.set_negative_electrode_submodel()
self.set_electrolyte_submodel()
Expand Down Expand Up @@ -118,7 +118,7 @@ def set_electrolyte_submodel(self):
"electrolyte conductivity"
] = pybamm.electrolyte_conductivity.Composite(self.param)
self.submodels["electrolyte diffusion"] = pybamm.electrolyte_diffusion.Full(
self.param, self.reactions
self.param
)

@property
Expand Down
8 changes: 1 addition & 7 deletions pybamm/models/submodels/base_submodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,7 @@ class BaseSubModel:
"""

def __init__(
self,
param,
domain=None,
reactions=None,
name="Unnamed submodel",
external=False,
self, param, domain=None, name="Unnamed submodel", external=False,
):
super().__init__()
self.param = param
Expand All @@ -67,7 +62,6 @@ def __init__(

self.domain = domain
self.set_domain_for_broadcast()
self.reactions = reactions
self.name = name

self.external = external
Expand Down
5 changes: 2 additions & 3 deletions pybamm/models/submodels/electrode/base_electrode.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ class BaseElectrode(pybamm.BaseSubModel):
**Extends:** :class:`pybamm.BaseSubModel`
"""

def __init__(self, param, domain, reactions=None, set_positive_potential=True):
super().__init__(param, domain, reactions)
def __init__(self, param, domain, set_positive_potential=True):
super().__init__(param, domain)
self.set_positive_potential = set_positive_potential

def _get_standard_potential_variables(self, phi_s):
Expand Down Expand Up @@ -190,4 +190,3 @@ def _get_standard_whole_cell_variables(self, variables):
)

return variables

4 changes: 2 additions & 2 deletions pybamm/models/submodels/electrode/ohm/base_ohm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ class BaseModel(BaseElectrode):
**Extends:** :class:`pybamm.electrode.BaseElectrode`
"""

def __init__(self, param, domain, reactions=None, set_positive_potential=True):
super().__init__(param, domain, reactions, set_positive_potential)
def __init__(self, param, domain, set_positive_potential=True):
super().__init__(param, domain, set_positive_potential)

def set_boundary_conditions(self, variables):

Expand Down
Loading