Issue 1500 - Print parameter info by submodel #3628
Conversation
|
Hello! This PR allows users to print parameters submodel-wise. The following is an example of it's working: CODE: import pybamm
model = pybamm.lithium_ion.SPM()
submodel = model.submodels["positive primary particle"]
model.print_parameter_info(by_submodel=True)OUTPUT: I would appreciate it if we could review this PR and merge it. Thank you! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3628 +/- ##
========================================
Coverage 99.60% 99.60%
========================================
Files 259 259
Lines 21287 21333 +46
========================================
+ Hits 21203 21249 +46
Misses 84 84 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @cringeyburger, changes looks good. Although to fix coverage you might first want to add some favorable test cases in test_read_input_parameters() in test_base_model. Please correct me if needed @agriyakhetarpal
|
Thanks @cringeyburger . This has revealed an issue with trying to print by submodel. At the moment However, the way this has been set up, Something like the below doesn't work since it just returns all the model variables. You don't know from outside of the submodel which fundamental variables it already expects to be defined. Using a default dict doesn't work since the variables need to be PyBaMM symbols with the correct domain etc. import pybamm
model = pybamm.lithium_ion.SPM()
submodel = model.submodels["negative primary open-circuit potential"]
submodel.print_parameter_info()
sumbodel_vars = submodel.get_fundamental_variables()
submodel.get_coupled_variables(model.variables)Maybe we should just make |
|
So, we need to change all instances of Can you check the code below and tell me if I am doing it right or not def get_coupled_variables(self, variables):
domain, Domain = self.domain_Domain
phase_name = self.phase_name
new_variables = {} # NEW CHANGE^^^^
if self.reaction == "lithium-ion main":
T = variables[f"{Domain} electrode temperature [K]"]
# For "particle-size distribution" models, take distribution version
# of c_s_surf that depends on particle size.
domain_options = getattr(self.options, domain)
if domain_options["particle size"] == "distribution":
sto_surf = variables[
f"{Domain} {phase_name}particle surface stoichiometry distribution"
]
# If variable was broadcast, take only the orphan
if isinstance(sto_surf, pybamm.Broadcast) and isinstance(
T, pybamm.Broadcast
):
sto_surf = sto_surf.orphans[0]
T = T.orphans[0]
T = pybamm.PrimaryBroadcast(T, [f"{domain} particle size"])
else:
sto_surf = variables[
f"{Domain} {phase_name}particle surface stoichiometry"
]
# If variable was broadcast, take only the orphan
if isinstance(sto_surf, pybamm.Broadcast) and isinstance(
T, pybamm.Broadcast
):
sto_surf = sto_surf.orphans[0]
T = T.orphans[0]
ocp_surf = self.phase_param.U(sto_surf, T)
dUdT = self.phase_param.dUdT(sto_surf)
# Bulk OCP is from the average SOC and temperature
sto_bulk = variables[f"{Domain} electrode {phase_name}stoichiometry"]
T_bulk = pybamm.xyz_average(pybamm.size_average(T))
ocp_bulk = self.phase_param.U(sto_bulk, T_bulk)
elif self.reaction == "lithium metal plating":
T = variables[f"{Domain} electrode temperature [K]"]
ocp_surf = 0 * T
ocp_bulk = pybamm.Scalar(0)
dUdT = 0 * T
elif self.reaction == "lead-acid main":
c_e = variables[f"{Domain} electrolyte concentration [mol.m-3]"]
# If c_e was broadcast, take only the orphan
if isinstance(c_e, pybamm.Broadcast):
c_e = c_e.orphans[0]
ocp_surf = self.phase_param.U(c_e, self.param.T_init)
dUdT = pybamm.Scalar(0)
# Bulk OCP is from the average concentration and temperature
c_e_av = variables["X-averaged electrolyte concentration [mol.m-3]"]
ocp_bulk = self.phase_param.U(c_e_av, self.param.T_init)
elif self.reaction == "lead-acid oxygen":
ocp_surf = self.param.U_Ox
ocp_bulk = self.param.U_Ox
dUdT = pybamm.Scalar(0)
# NEW CHANGE^^^
new_variables.update(
self._get_standard_ocp_variables(ocp_surf, ocp_bulk, dUdT)
)
# NEW CHANGE^^^^
return new_variablesLater, I think we can access it like this? import pybamm
model = pybamm.lithium_ion.SPM()
submodel = model.submodels["negative primary open-circuit potential"]
new_variables = submodel.get_coupled_variables(model.variables)
model.variables.update(new_variables)
print("New variables introduced by the submodel:", new_variables) |
|
Changing |
|
Also, it might be nice to have an option where only the parameters that are unique to that submodel are printed. Otherwise, a lot of submodels are going to have a lot of common parameters that appear everywhere (e.g. electrode height). But it might be difficult to decide how "unique" a parameter has to be - e.g. what if it only appears in two submodels? |
True, I will give this a try.
Hmm.. we may need to parse through and keep a track of all parameters in such a case. I will get to you shortly. |
Yes, but then you need to figure out which variables to pass to model = pybamm.lithium_ion.SPM()
all_fundamental_vars = {}
for submodel in model.submodels.values():
all_fundamental_vars.update(submodel.get_fundamental_variables())
sumbodel_fundamental_vars = submodel.get_fundamental_variables()
updated_vars = submodel.get_coupled_variables(all_fundamental_vars)also fails. If the behaviour to The solution that avoids changing In any case, we should also throw an error if users try to use |
…ectly on any submodel.
|
Does this solve it? def build_coupled_variables(self):
# Note: pybamm will try to get the coupled variables for the submodels in the
# order they are set by the user. If this fails for a particular submodel,
# return to it later and try again. If setting coupled variables fails and
# there are no more submodels to try, raise an error.
submodels = list(self.submodels.keys())
count = 0
# For this part the FuzzyDict of variables is briefly converted back into a
# normal dictionary for speed with KeyErrors
self._variables = dict(self._variables)
while len(submodels) > 0:
count += 1
for submodel_name, submodel in self.submodels.items():
if submodel_name in submodels:
pybamm.logger.debug(
"Getting coupled variables for {} submodel ({})".format(
submodel_name, self.name
)
)
try:
# ----------NEW
submodel_coupled_vars = submodel.get_coupled_variables(self.variables)
# ----------NEW
self.variables[submodel_name] = submodel_coupled_vars
submodels.remove(submodel_name)
except KeyError as key:
if len(submodels) == 1 or count == 100:
# no more submodels to try
raise pybamm.ModelError(
"Missing variable for submodel '{}': {}.\n".format(
submodel_name, key
)
+ "Check the selected "
"submodels provide all of the required variables."
)
else:
# try setting coupled variables on next loop through
pybamm.logger.debug(
f"Can't find {key}, trying other submodels first"
)
# Convert variables back into FuzzyDict
self.variables = pybamm.FuzzyDict(self._variables)I think we will end up with a dict, self.variables like this: {
'submodel1': {'coupled_var1': value1, 'coupled_var2': value2, ...},
'submodel2': {'coupled_var3': value3, 'coupled_var4': value4, ...},
...
}I am a bit confused as what to do now. |
|
It would be great if you could commit and push this change, @cringeyburger – doing that allows us to see the diff and track what lines were added or removed. |
… added by get_coupled_variables in model.variables
|
I think I understand where I did the mistake. I didn't account for this:
So I think what I should do, is keep a copy of What I did was simply add the result of updating So in reality, {
'submodel1': {'pre-existing model variables': model_value, 'coupled_var1': value1, 'coupled_var2': value2},
'submodel2': {'pre-existing model variables': model_value, 'coupled_var1': value1, 'coupled_var2': value2, 'coupled_var3': value3, 'coupled_var4': value4},
...
}where I need something like this: {
'submodel1': {'coupled_var1': value1, 'coupled_var2': value2, ...},
'submodel2': {'coupled_var3': value3, 'coupled_var4': value4, ...},
...
}Am I right regarding this? |
… added by get_coupled_variables in model.variables V2
|
I think this should work now. import pybamm
model = pybamm.lithium_ion.SPM()
model.print_parameter_info()This is the output I got: Traceback (most recent call last):
File "/home/yukinatsu/code/PyBaMM/pybamm/models/base_model.py", line 584, in build_coupled_variables
submodel_coupled_result = submodel.get_coupled_variables(self.variables)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/yukinatsu/code/PyBaMM/pybamm/models/submodels/interface/kinetics/inverse_kinetics/inverse_butler_volmer.py", line 39, in get_coupled_variables
j_tot_av, a_j_tot_av = self._get_average_total_interfacial_current_density(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/yukinatsu/code/PyBaMM/pybamm/models/submodels/interface/base_interface.py", line 185, in _get_average_total_interfacial_current_density
i_boundary_cc = variables["Current collector current density [A.m-2]"]
~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: 'Current collector current density [A.m-2]'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/yukinatsu/code/PyBaMM/mydir/test.py", line 3, in <module>
model = pybamm.lithium_ion.SPM()
^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/yukinatsu/code/PyBaMM/pybamm/models/full_battery_models/lithium_ion/spm.py", line 44, in __init__
self.set_submodels(build)
File "/home/yukinatsu/code/PyBaMM/pybamm/models/full_battery_models/lithium_ion/base_lithium_ion_model.py", line 59, in set_submodels
self.build_model()
File "/home/yukinatsu/code/PyBaMM/pybamm/models/full_battery_models/base_battery_model.py", line 1082, in build_model
self._build_model()
File "/home/yukinatsu/code/PyBaMM/pybamm/models/base_model.py", line 655, in _build_model
self.build_coupled_variables()
File "/home/yukinatsu/code/PyBaMM/pybamm/models/base_model.py", line 591, in build_coupled_variables
raise pybamm.ModelError(
pybamm.expression_tree.exceptions.ModelError: Missing variable for submodel 'negative interface': 'Current collector current density [A.m-2]'.
Check the selected submodels provide all of the required variables.
ERROR conda.cli.main_run:execute(49): `conda run python /home/yukinatsu/code/PyBaMM/mydir/test.py` failed. (See above for error)Just to provide info, I ran the code without any changes from this PR except Any idea why this is happening? |
pybamm/models/base_model.py
Outdated
| submodel.get_coupled_variables(self.variables) | ||
| ) | ||
| submodel_coupled_vars = submodel.get_coupled_variables(self.variables) | ||
| self.variables[submodel_name] = submodel_coupled_vars |
There was a problem hiding this comment.
variables is already a dict with keys corresponding to variable names and values corresponding to their symbols. you'll need to store the coupled variables that each submodel defines in a new object.
i don't know if it makes sense to do this when building the model, or if it should be done separately when getting the parameter info. does it make a difference if we store e.g. self.variables and self.variables_by_submodel where the latter is a dict of dicts (e.g. {"submodel 1": {"var1": x, ...}, ...}
There was a problem hiding this comment.
From what I know, there isn't much benefit to make a separate dict. other from keeping info clean and sorted + avoid searching a large dictionary.
There was a problem hiding this comment.
i don't know if it makes sense to do this when building the model, or if it should be done separately when getting the parameter info
I think the latter is better. Accessing elements from a dictionary is usually very fast – even if a dict of dicts is used to print out the information, then it can be "normalised" with the name of a submodel as the key (i.e., print the values of the sub-dictionary, similar to how pandas can normalise JSON) to account for by_submodel=True.
If by_submodel is False, it can pretty-print the whole dict in a table like it currently does. This method might be better, but would also be harder.
There was a problem hiding this comment.
Okay.. so what should be done now?
There was a problem hiding this comment.
Should I add an object in build_coupled_variables, e.g. self.variables_by_submodel which has a structure:
{
'submodel1': {'coupled_var1': value1, 'coupled_var2': value2, ...},
'submodel2': {'coupled_var3': value3, 'coupled_var4': value4, ...},
...
}
The code for this would be the same as before:
try:
model_var_copy = self.variables.copy()
submodel_coupled_result = submodel.get_coupled_variables(self.variables)
self.variables_by_submodel[submodel_name] = {
key: submodel_coupled_result[key]
for key in submodel_coupled_result
if key not in model_var_copy
}
self.variables.update(
submodel.get_coupled_variables(self.variables)
)
submodels.remove(submodel_name)This stores both self.variables and self.variables_by_submodel.
Next, We can modify get_parameter_info to include by_submodel=False too.
In this way, if we set print_paramter_info(by_submodel=False), then get_parameter_info will print the usual way.
If we set print_paramter_info(by_submodel=True), then we change the parameter_info returned by get_parameter_info by submodel wise.
NOTE: I have been coding something for this. I was successful in getting the coupled variables. Now I am experimenting with parameter processing and trying to make the links between get_parameter_info and print_parameter_info better. Also trying to make the code efficient and cleaner to look at... Will commit soon
|
I have a question. Consider the code: import pybamm
def write_info_to_file(file_path, info):
with open(file_path, 'w') as file:
for key, value in info.items():
file.write(f"{key}: {value}\n")
model = pybamm.lithium_ion.SPM()
info = model.get_parameter_info(by_submodel=True)
output_file_path = 'output.py'
write_info_to_file(output_file_path, model.variables_by_submodel)This basically prints the output in another file, you only need to concern with what output is printed. NOTE: I have modified Consider this submodel variables (fundamental + coupled): As you can notice, there is no So when I run the code, but output-ing the OUTPUT: So the question is, Is this what is required? I mean, are we targeting only those three categories, i.e. NOTE: I will commit the version where we only filter for these three categories of parameters. If there is any change required, I will modify it quickly afterwards. |
… name as key and its variables as value. Modified `get_parameter_info` to parse through the submodels' variables and give out `parameter_info` in `by_submodel=True` condition Modified `print_parameter_info` to print `parameter_info` of a model with an option to print submodel wise using `by_submodel=True`
|
I have worked out a solution to this problem. The method below works. Then I modified It has a structure as follows: I then modified Then I modified NOTE:
I have to work on this. Also, I still need to create test cases for the PR. I will do it soon (actually, it is my first time creating tests)
It would be great if this is cleared so we can move further. |
|
Here is an example of the output: import pybamm
model = pybamm.lithium_ion.SPM()
model.print_parameter_info(by_submodel=True)OUTPUT: New parameters are being printed, so this is a good attempt towards our desired solution. |
|
Hi! I have been trying to code the test for
|
cringeyburger
requested review from
brosaplanella,
DrSOKane and
TomTranter
as code owners
…r_info" is used directly on a submodel
|
Input parameters are just a placeholder for the other types of parameters, which basically allows you to plug them in at the solving stage. Hence, there would be none appearing in the models so don't worry about them. Does this sort the issue? |
|
Since you said that no |
|
For testing you can make a model from submodels like this model = pybamm.BaseModel()
a = pybamm.InputParameter("a")
b = pybamm.InputParameter("b", "test")
c = pybamm.InputParameter("c")
d = pybamm.InputParameter("d")
e = pybamm.InputParameter("e")
f = pybamm.InputParameter("f")
g = pybamm.Parameter("g")
h = pybamm.Parameter("h")
i = pybamm.Parameter("i")
u = pybamm.Variable("u")
v = pybamm.Variable("v")
sub1 = pybamm.BaseSubModel(None)
sub1.rhs = {u: -u * a}
sub1.initial_conditions = {u: c}
sub1.variables = {"u": u}
sub1.boundary_conditions = {
u: {"left": (g, "Dirichlet"), "right": (0, "Neumann")},
}
sub2 = pybamm.BaseSubModel(None)
sub2.algebraic = {v: v - b}
sub2.variables = {"v":v, "v+f+i": v + f + i}
sub2.initial_conditions = {v: d}
sub2.boundary_conditions = {
v: {"left": (0, "Dirichlet"), "right": (h, "Neumann")},
}
model.submodels = {"sub1": sub1, "sub2": sub2}
model.events = [pybamm.Event("u=e", u - e)]
model.build_model() |
|
With this commit, most of the code is covered by tests. The only thing left is for var_name, var_symbol in submodel_vars.items():
if isinstance(var_symbol, pybamm.Parameter):
submodel_info[var_name] = (var_symbol, "Parameter")
elif isinstance(var_symbol, pybamm.InputParameter):
if not var_symbol.domain:
submodel_info[var_name] = (var_symbol, "InputParameter")
else:
submodel_info[var_name] = (
var_symbol,
f"InputParameter in {var_symbol.domain}",
)
elif isinstance(var_symbol, pybamm.FunctionParameter):
input_names = "', '".join(var_symbol.input_names)
submodel_info[var_name] = (
var_symbol,
f"FunctionParameter with inputs(s) '{input_names}'",
)in I figured out the issue. The submodels do not have I have been trying to extend the code given by @rtimms by adding fundamental and coupled variables, but I need help. I would be grateful if someone could guide me on how to add fundamental and coupled variables to Thank youu |
|
@cringeyburger you could make the submodels into classes so they use the fundamental and coupled variables, e.g. class SubModel1(pybamm.BaseSubModel):
def get_fundamental_variables(self):
u = pybamm.Variable("u")
variables = {"u": u}
return variables
def get_coupled_variables(self, variables):
u = variables["u"]
v = variables["v"]
w = u * v
variables.update({"w": w})
return variables
def set_rhs(self, variables):
a = pybamm.InputParameter("a")
u = variables["u"]
self.rhs = {u: -u * a}
def set_boundary_conditions(self, variables):
g = pybamm.Parameter("g")
self.boundary_conditions = {
u: {"left": (g, "Dirichlet"), "right": (0, "Neumann")},
}
def set_initial_conditions(self, variables):
c = pybamm.InputParameter("c")
u = variables["u"]
self.initial_conditions = {u: c}
def set_events(self, variables):
e = pybamm.InputParameter("e")
u = variables["u"]
self.events = [pybamm.Event("u=e", u - e)]
class SubModel2(pybamm.BaseSubModel):
def get_fundamental_variables(self):
v = pybamm.Variable("v")
f = pybamm.InputParameter("f")
i = pybamm.Parameter("i")
variables = {"v": v, "v+f+i": v + f + i}
return variables
def set_rhs(self, variables):
b = pybamm.InputParameter("b", "test")
v = variables["v"]
self.rhs = {v: v - b}
def set_boundary_conditions(self, variables):
h = pybamm.Parameter("h")
v = variables["v"]
self.boundary_conditions = {
v: {"left": (0, "Dirichlet"), "right": (h, "Neumann")},
}
def set_initial_conditions(self, variables):
c = pybamm.InputParameter("c")
v = variables["v"]
self.initial_conditions = {v: c}
sub1 = SubModel1(None)
sub2 = SubModel2(None)
model = pybamm.BaseModel()
model.submodels = {"sub1": sub1, "sub2": sub2}
model.build_model() |
|
With the latest commit, I am getting 100% coverage on my code. I especially give thanks to everyone who helped me on this issue. I learned a lot during this issue. |
|
I noticed the |
|
It's not a big issue and we have been considering reducing how much Lychee runs or reducing the number of links across the codebase if necessary, since a lot of them are unneeded and can contribute to link rot. From the job summary it looks like Stack Overflow is hitting back against automated link checkers, so if you want to help out – maybe you can add a user agent to bypass this and test the changes on your fork, or replace those links with non-SO alternatives that explain the same thing (wherever it's possible, and if there are any – I don't expect there will be many). I wouldn't recommend removing the links because they were added as notes for certain functionality, for both accreditation and further reference. |
|
Unfortunately, I don't think I can fix this issue. I had set up a fake User-Agent in the But the way my university's ethernet and wifi are set up (basically highly restricted, blocked ports and more), all of the links are repeatedly failed by Act, either by connection reset or unexpected EOF. |
|
I don't usually use |
|
@cringeyburger, was this in error? |
|
Yes! I don't know what how or what happened. I am so sorry!! |
Description
Further improved on "print_parameter_info" by implementing "by_submodel" feature which allow users to print parameters sub-model wise.
Fixes #1500
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all(or$ nox -s tests)$ python run-tests.py --doctest(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick(or$ nox -s quick).Further checks: