Issue 3361 - Improve print_parameter_info functionality #3584

cringeyburger · 2023-12-03T17:49:34Z

Description

Implemented "parameter_info" method to extract parameters and modified "print_parameter_info" to print the required parameters.

Fixes #3361

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.

New feature (non-breaking change which adds functionality)
Optimization (back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)

Key checklist:

No style issues: $ 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)
All tests pass: $ python run-tests.py --all (or $ nox -s tests)
The documentation builds: $ 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:

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

…arameter_info

cringeyburger · 2023-12-03T17:54:49Z

Hello! I would appreciate it if someone could review this code.
Thanks to @agriyakhetarpal for suggesting changes in the feature. I would be happy to make any requested changes.

PS: I need to build some test cases. I would be grateful if someone could guide me on how to do it.

cringeyburger · 2023-12-03T17:57:48Z

NOTE: I have also tried coding something for #1500 and would be PR-ing it as soon as this PR is resolved.

codecov · 2023-12-04T23:40:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4ad2853) 99.59% compared to head (55a70da) 99.59%.
Report is 5 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3584   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          258      258           
  Lines        20755    20778   +23     
========================================
+ Hits         20670    20693   +23     
  Misses          85       85

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinsulzer

Thanks for taking this on. A couple of stylistic changes suggested. I'm not sure what @brosaplanella had in mind, but it would be easier to just take the existing logic from print_parameter_info to make a new function called get_parameter_info which returns a list (instead of printing) - or maybe a dictionary of {parameter: details}, and then in print_parameter_info just do

def print_parameter_info(self):
    info = self.get_parameter_info()
    print(info) # or something else depending on the return type

pybamm/models/base_model.py

… and modified "print_parameter_info" to print the dictionary

cringeyburger · 2023-12-05T13:04:43Z

Hey @tinosulzer, I have implemented the feature following the logic you provided. It would be great if you could review it. Thanks!

valentinsulzer

Looks good to me, is this what you had in mind @brosaplanella ?

pybamm/models/base_model.py

agriyakhetarpal

Thanks @cringeyburger, looks good to me too. It looks like the print_parameter_info method is not documented in the API (see #3392), but we have a PR addressing that. I have a small suggestion about the readability of the output

pybamm/models/base_model.py

brosaplanella

Thanks for picking it up @cringeyburger! Looks good to me once the other comments are addressed.

pybamm/models/base_model.py

…e parameters instead of their names

…ameter_info" (initial version)

cringeyburger · 2023-12-07T03:51:27Z

Thanks for waiting! The string formatting took a lot of time.
This is how "print_parameter_info" prints now:

========================================================================================================================================================================================================================================================================================
| Parameter                                                                                  | Type of parameter                                                                          | Parameter inputs                                                                           |
========================================================================================================================================================================================================================================================================================
| Faraday constant [C.mol-1]                                                                 | Parameter                                                                                  |                                                                                            |
| Number of electrodes connected in parallel to make a cell                                  | Parameter                                                                                  |                                                                                            |
| Maximum concentration in positive electrode [mol.m-3]                                      | Parameter                                                                                  |                                                                                            |
| Positive electrode thickness [m]                                                           | Parameter                                                                                  |                                                                                            |
| Electrode height [m]                                                                       | Parameter                                                                                  |                                                                                            |
| Electrode width [m]                                                                        | Parameter                                                                                  |                                                                                            |
| Initial concentration in positive electrode [mol.m-3]                                      | FunctionParameter with inputs(s) 'Radial distance (r) [m]', 'Through-cell distance (x) [m] | 'Radial distance (r) [m]', 'Through-cell distance (x) [m]'                                 |
|                                                                                            | '                                                                                          |                                                                                            |
| Positive electrode diffusivity [m2.s-1]                                                    | FunctionParameter with inputs(s) 'Positive particle stoichiometry', 'Temperature [K]'      | 'Positive particle stoichiometry', 'Temperature [K]'                                       |
| Positive electrode active material volume fraction                                         | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                           | 'Through-cell distance (x) [m]'                                                            |
| Current function [A]                                                                       | FunctionParameter with inputs(s) 'Time[s]'                                                 | 'Time[s]'                                                                                  |
| Positive particle radius [m]                                                               | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                           | 'Through-cell distance (x) [m]'                                                            |
========================================================================================================================================================================================================================================================================================

I think improvements can be made regarding the dynamicness of the table. Otherwise, it should suffice. If there are any requests, I would happily take them in.

pybamm/models/base_model.py

CHANGELOG.md

…meter_info" and "print_parameter_info"

…meter_info" in CHANGELOG.md

cringeyburger · 2023-12-07T12:53:55Z

Regarding the dynamicity of the print_parameter_info method, I have adjusted the logic and it works.
Following are two examples of code and their output:
A) CODE

import pybamm

model = pybamm.lithium_ion.SPM()
submodel = model.submodels["positive primary particle"]
submodel.print_parameter_info()

OUTPUT

| Parameter                                                 | Type of parameter                                                                           |
| ========================================================= | =========================================================================================== |
| Maximum concentration in positive electrode [mol.m-3]     | Parameter                                                                                   |
| Electrode height [m]                                      | Parameter                                                                                   |
| Number of electrodes connected in parallel to make a cell | Parameter                                                                                   |
| Faraday constant [C.mol-1]                                | Parameter                                                                                   |
| Electrode width [m]                                       | Parameter                                                                                   |
| Positive electrode thickness [m]                          | Parameter                                                                                   |
| Current function [A]                                      | FunctionParameter with inputs(s) 'Time[s]'                                                  |
| Positive particle radius [m]                              | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                            |
| Positive electrode active material volume fraction        | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                            |
| Initial concentration in positive electrode [mol.m-3]     | FunctionParameter with inputs(s) 'Radial distance (r) [m]', 'Through-cell distance (x) [m]' |
| Positive electrode diffusivity [m2.s-1]                   | FunctionParameter with inputs(s) 'Positive particle stoichiometry', 'Temperature [K]'       |

B) CODE

import pybamm

model = pybamm.lithium_ion.SPM()
submodel = model.submodels["positive primary particle"]
model.print_parameter_info()

OUTPUT

| Parameter                                                 | Type of parameter                                                                                                                                                                                           |
| ========================================================= | =========================================================================================================================================================================================================== |
| Electrode height [m]                                      | Parameter                                                                                                                                                                                                   |
| Faraday constant [C.mol-1]                                | Parameter                                                                                                                                                                                                   |
| Negative electrode Bruggeman coefficient (electrolyte)    | Parameter                                                                                                                                                                                                   |
| Separator thickness [m]                                   | Parameter                                                                                                                                                                                                   |
| Negative electrode thickness [m]                          | Parameter                                                                                                                                                                                                   |
| Positive electrode thickness [m]                          | Parameter                                                                                                                                                                                                   |
| Nominal cell capacity [A.h]                               | Parameter                                                                                                                                                                                                   |
| Initial concentration in electrolyte [mol.m-3]            | Parameter                                                                                                                                                                                                   |
| Number of cells connected in series to make a battery     | Parameter                                                                                                                                                                                                   |
| Upper voltage cut-off [V]                                 | Parameter                                                                                                                                                                                                   |
| Lower voltage cut-off [V]                                 | Parameter                                                                                                                                                                                                   |
| Maximum concentration in positive electrode [mol.m-3]     | Parameter                                                                                                                                                                                                   |
| Ideal gas constant [J.K-1.mol-1]                          | Parameter                                                                                                                                                                                                   |
| Separator Bruggeman coefficient (electrolyte)             | Parameter                                                                                                                                                                                                   |
| Positive electrode Bruggeman coefficient (electrolyte)    | Parameter                                                                                                                                                                                                   |
| Negative electrode Bruggeman coefficient (electrode)      | Parameter                                                                                                                                                                                                   |
| Maximum concentration in negative electrode [mol.m-3]     | Parameter                                                                                                                                                                                                   |
| Reference temperature [K]                                 | Parameter                                                                                                                                                                                                   |
| Number of electrodes connected in parallel to make a cell | Parameter                                                                                                                                                                                                   |
| Positive electrode Bruggeman coefficient (electrode)      | Parameter                                                                                                                                                                                                   |
| Electrode width [m]                                       | Parameter                                                                                                                                                                                                   |
| Positive electrode OCP entropic change [V.K-1]            | FunctionParameter with inputs(s) 'Positive particle stoichiometry', 'Maximum positive particle surface concentration [mol.m-3]'                                                                             |
| Positive electrode OCP entropic change [V.K-1]            | FunctionParameter with inputs(s) 'Positive particle stoichiometry', 'Maximum positive particle surface concentration [mol.m-3]'                                                                             |
| Positive electrode diffusivity [m2.s-1]                   | FunctionParameter with inputs(s) 'Positive particle stoichiometry', 'Temperature [K]'                                                                                                                       |
| Negative electrode exchange-current density [A.m-2]       | FunctionParameter with inputs(s) 'Electrolyte concentration [mol.m-3]', 'Negative particle surface concentration [mol.m-3]', 'Maximum negative particle surface concentration [mol.m-3]', 'Temperature [K]' |
| Negative electrode OCP entropic change [V.K-1]            | FunctionParameter with inputs(s) 'Negative particle stoichiometry', 'Maximum negative particle surface concentration [mol.m-3]'                                                                             |
| Positive electrode porosity                               | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                                                                                                                                            |
| Negative electrode active material volume fraction        | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                                                                                                                                            |
| Current function [A]                                      | FunctionParameter with inputs(s) 'Time[s]'                                                                                                                                                                  |
| Initial concentration in negative electrode [mol.m-3]     | FunctionParameter with inputs(s) 'Radial distance (r) [m]', 'Through-cell distance (x) [m]'                                                                                                                 |
| Negative particle radius [m]                              | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                                                                                                                                            |
| Negative electrode porosity                               | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                                                                                                                                            |
| Positive particle radius [m]                              | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                                                                                                                                            |
| Positive electrode OCP [V]                                | FunctionParameter with inputs(s) 'Positive particle stoichiometry'                                                                                                                                          |
| Positive electrode OCP [V]                                | FunctionParameter with inputs(s) 'Positive particle stoichiometry'                                                                                                                                          |
| Negative electrode diffusivity [m2.s-1]                   | FunctionParameter with inputs(s) 'Negative particle stoichiometry', 'Temperature [K]'                                                                                                                       |
| Separator porosity                                        | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                                                                                                                                            |
| Negative particle radius [m]                              | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                                                                                                                                            |
| Positive electrode active material volume fraction        | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                                                                                                                                            |
| Positive electrode OCP entropic change [V.K-1]            | FunctionParameter with inputs(s) 'Positive particle stoichiometry', 'Maximum positive particle surface concentration [mol.m-3]'                                                                             |
| Positive electrode exchange-current density [A.m-2]       | FunctionParameter with inputs(s) 'Electrolyte concentration [mol.m-3]', 'Positive particle surface concentration [mol.m-3]', 'Maximum positive particle surface concentration [mol.m-3]', 'Temperature [K]' |
| Negative electrode OCP entropic change [V.K-1]            | FunctionParameter with inputs(s) 'Negative particle stoichiometry', 'Maximum negative particle surface concentration [mol.m-3]'                                                                             |
| Negative electrode OCP entropic change [V.K-1]            | FunctionParameter with inputs(s) 'Negative particle stoichiometry', 'Maximum negative particle surface concentration [mol.m-3]'                                                                             |
| Negative electrode OCP [V]                                | FunctionParameter with inputs(s) 'Negative particle stoichiometry'                                                                                                                                          |
| Positive particle radius [m]                              | FunctionParameter with inputs(s) 'Through-cell distance (x) [m]'                                                                                                                                            |
| Initial concentration in positive electrode [mol.m-3]     | FunctionParameter with inputs(s) 'Radial distance (r) [m]', 'Through-cell distance (x) [m]'                                                                                                                 |
| Negative electrode OCP [V]                                | FunctionParameter with inputs(s) 'Negative particle stoichiometry'                                                                                                                                          |
| Ambient temperature [K]                                   | FunctionParameter with inputs(s) 'Distance across electrode width [m]', 'Distance across electrode height [m]', 'Time [s]'                                                                                  |

agriyakhetarpal

The changes look great to me – thanks @cringeyburger! Could you merge the changes from the develop branch and resolve the merge conflicts?

…unctionality

cringeyburger · 2023-12-07T16:59:54Z

I think we are set to merge the PR. Thanks for helping me through out the issue! Also, I would be pulling another one for #1500, please look forward to it! 👋

pybamm/models/base_model.py

valentinsulzer

Happy to merge once tests pass

cringeyburger · 2023-12-07T21:03:23Z

Hii, I just had a small doubt. You can observe that the PR failed a few tests. The question is, why? like the PR doesn't have anything to do with the solvers, so shouldn't the performance remain same?

valentinsulzer · 2023-12-07T23:06:10Z

Don't worry about those test and benchmark errors, however it looks like the example that is failing is legitimately broken

agriyakhetarpal · 2023-12-08T06:01:34Z

I think the example notebook is failing due to the changes made here, it works and passes on other PRs. The notebook should be re-run and the outputs should be updated to get the tabular format for the parameters wherever necessary.

The failing code block is this one:

spm = pybamm.lithium_ion.SPM()
{k: v for k,v in spm.default_parameter_values.items() if k in spm._parameter_info}

Another thing to be noted here is that we have previously been using a private API (i.e., _parameter_info) to get the list of default parameters for a model. My recommendation is that this should be updated to use a public, user-facing method instead so as not to mislead users.

cringeyburger · 2023-12-08T14:34:01Z

Is there something I can help with? I would like to know the ins and outs of the project better.

agriyakhetarpal · 2023-12-09T14:48:43Z

Is there something I can help with? I would like to know the ins and outs of the project better.

Yes, could you run and fix the notebook locally and push the newer outputs? I think it arises because _parameter_info is now None and thus cannot be iterated over, but I checked and spm.default_parameter_values is returning an identical output as the current dictionary comprehension, so I'm not sure why we were even using a private attribute without a getter. I think default_parameter_values should be fine

cringeyburger · 2023-12-10T10:45:42Z

I went through the notebook, ran some things and noticed a few things.

A) The problem with simply removing ._parameter_info is that, the cells which are associated with printing parameters in the notebook, will print unnecessary parameters.

EXAMPLE:

CODE without _parameter_info:--> In both Issue 3361 - Improve print_parameter_info functionality branch and develop branch

{k: v for k,v in spm.default_parameter_values.items()}

OUTPUT:

{'Ideal gas constant [J.K-1.mol-1]': 8.314462618,
 'Faraday constant [C.mol-1]': 96485.33212,
 'Boltzmann constant [J.K-1]': 1.380649e-23,
 'Electron charge [C]': 1.602176634e-19,
 'Ratio of lithium moles to SEI moles': 2.0,
 'Inner SEI reaction proportion': 0.5,
 'Inner SEI partial molar volume [m3.mol-1]': 9.585e-05,
 'Outer SEI partial molar volume [m3.mol-1]': 9.585e-05,
 'SEI growth transfer coefficient': 0.5,
 'SEI reaction exchange current density [A.m-2]': 1.5e-07,
 'SEI resistivity [Ohm.m]': 200000.0,
 'Outer SEI solvent diffusivity [m2.s-1]': 2.5000000000000002e-22,
 'Bulk solvent concentration [mol.m-3]': 2636.0,
 'Inner SEI open-circuit potential [V]': 0.1,
 'Outer SEI open-circuit potential [V]': 0.8,
 'Inner SEI electron conductivity [S.m-1]': 8.95e-14,
 'Inner SEI lithium interstitial diffusivity [m2.s-1]': 1e-20,
 'Lithium interstitial reference concentration [mol.m-3]': 15.0,
 'Initial inner SEI thickness [m]': 2.5e-09,
 'Initial outer SEI thickness [m]': 2.5e-09,
 'EC initial concentration in electrolyte [mol.m-3]': 4541.0,
 'EC diffusivity [m2.s-1]': 2e-18,
 'SEI kinetic rate constant [m.s-1]': 1e-12,
 'SEI open-circuit potential [V]': 0.4,
 'SEI growth activation energy [J.mol-1]': 0.0,
 'Negative electrode reaction-driven LAM factor [m3.mol-1]': 0.0,
 'Positive electrode reaction-driven LAM factor [m3.mol-1]': 0.0,
 'Negative current collector thickness [m]': 2.5e-05,
 'Negative electrode thickness [m]': 0.0001,
 'Separator thickness [m]': 2.5e-05,
 'Positive electrode thickness [m]': 0.0001,
 'Positive current collector thickness [m]': 2.5e-05,
 'Electrode height [m]': 0.137,
 'Electrode width [m]': 0.207,
 'Negative tab width [m]': 0.04,
 'Negative tab centre y-coordinate [m]': 0.06,
 'Negative tab centre z-coordinate [m]': 0.137,
 'Positive tab width [m]': 0.04,
 'Positive tab centre y-coordinate [m]': 0.147,
 'Positive tab centre z-coordinate [m]': 0.137,
 'Cell cooling surface area [m2]': 0.0569,
 'Cell volume [m3]': 7.8e-06,
 'Negative current collector conductivity [S.m-1]': 59600000.0,
 'Positive current collector conductivity [S.m-1]': 35500000.0,
 'Negative current collector density [kg.m-3]': 8954.0,
 'Positive current collector density [kg.m-3]': 2707.0,
 'Negative current collector specific heat capacity [J.kg-1.K-1]': 385.0,
 'Positive current collector specific heat capacity [J.kg-1.K-1]': 897.0,
 'Negative current collector thermal conductivity [W.m-1.K-1]': 401.0,
 'Positive current collector thermal conductivity [W.m-1.K-1]': 237.0,
 'Nominal cell capacity [A.h]': 0.680616,
 'Current function [A]': 0.680616,
 'Contact resistance [Ohm]': 0,
 'Negative electrode conductivity [S.m-1]': 100.0,
 'Maximum concentration in negative electrode [mol.m-3]': 24983.2619938437,
 'Negative electrode diffusivity [m2.s-1]': <function pybamm.input.parameters.lithium_ion.Marquis2019.graphite_mcmb2528_diffusivity_Dualfoil1998(sto, T)>,
 'Negative electrode OCP [V]': <function pybamm.input.parameters.lithium_ion.Marquis2019.graphite_mcmb2528_ocp_Dualfoil1998(sto)>,
 'Negative electrode porosity': 0.3,
 'Negative electrode active material volume fraction': 0.6,
 'Negative particle radius [m]': 1e-05,
 'Negative electrode Bruggeman coefficient (electrolyte)': 1.5,
 'Negative electrode Bruggeman coefficient (electrode)': 1.5,
 'Negative electrode charge transfer coefficient': 0.5,
 'Negative electrode double-layer capacity [F.m-2]': 0.2,
 'Negative electrode exchange-current density [A.m-2]': <function pybamm.input.parameters.lithium_ion.Marquis2019.graphite_electrolyte_exchange_current_density_Dualfoil1998(c_e, c_s_surf, c_s_max, T)>,
 'Negative electrode density [kg.m-3]': 1657.0,
 'Negative electrode specific heat capacity [J.kg-1.K-1]': 700.0,
 'Negative electrode thermal conductivity [W.m-1.K-1]': 1.7,
 'Negative electrode OCP entropic change [V.K-1]': <function pybamm.input.parameters.lithium_ion.Marquis2019.graphite_entropic_change_Moura2016(sto, c_s_max)>,
 'Positive electrode conductivity [S.m-1]': 10.0,
 'Maximum concentration in positive electrode [mol.m-3]': 51217.9257309275,
 'Positive electrode diffusivity [m2.s-1]': <function pybamm.input.parameters.lithium_ion.Marquis2019.lico2_diffusivity_Dualfoil1998(sto, T)>,
 'Positive electrode OCP [V]': <function pybamm.input.parameters.lithium_ion.Marquis2019.lico2_ocp_Dualfoil1998(sto)>,
 'Positive electrode porosity': 0.3,
 'Positive electrode active material volume fraction': 0.5,
 'Positive particle radius [m]': 1e-05,
 'Positive electrode Bruggeman coefficient (electrolyte)': 1.5,
 'Positive electrode Bruggeman coefficient (electrode)': 1.5,
 'Positive electrode charge transfer coefficient': 0.5,
 'Positive electrode double-layer capacity [F.m-2]': 0.2,
 'Positive electrode exchange-current density [A.m-2]': <function pybamm.input.parameters.lithium_ion.Marquis2019.lico2_electrolyte_exchange_current_density_Dualfoil1998(c_e, c_s_surf, c_s_max, T)>,
 'Positive electrode density [kg.m-3]': 3262.0,
 'Positive electrode specific heat capacity [J.kg-1.K-1]': 700.0,
 'Positive electrode thermal conductivity [W.m-1.K-1]': 2.1,
 'Positive electrode OCP entropic change [V.K-1]': <function pybamm.input.parameters.lithium_ion.Marquis2019.lico2_entropic_change_Moura2016(sto, c_s_max)>,
 'Separator porosity': 1.0,
 'Separator Bruggeman coefficient (electrolyte)': 1.5,
 'Separator density [kg.m-3]': 397.0,
 'Separator specific heat capacity [J.kg-1.K-1]': 700.0,
 'Separator thermal conductivity [W.m-1.K-1]': 0.16,
 'Initial concentration in electrolyte [mol.m-3]': 1000.0,
 'Cation transference number': 0.4,
 'Thermodynamic factor': 1.0,
 'Electrolyte diffusivity [m2.s-1]': <function pybamm.input.parameters.lithium_ion.Marquis2019.electrolyte_diffusivity_Capiglia1999(c_e, T)>,
 'Electrolyte conductivity [S.m-1]': <function pybamm.input.parameters.lithium_ion.Marquis2019.electrolyte_conductivity_Capiglia1999(c_e, T)>,
 'Reference temperature [K]': 298.15,
 'Ambient temperature [K]': 298.15,
 'Negative current collector surface heat transfer coefficient [W.m-2.K-1]': 0.0,
 'Positive current collector surface heat transfer coefficient [W.m-2.K-1]': 0.0,
 'Negative tab heat transfer coefficient [W.m-2.K-1]': 10.0,
 'Positive tab heat transfer coefficient [W.m-2.K-1]': 10.0,
 'Edge heat transfer coefficient [W.m-2.K-1]': 0.3,
 'Total heat transfer coefficient [W.m-2.K-1]': 10.0,
 'Number of electrodes connected in parallel to make a cell': 1.0,
 'Number of cells connected in series to make a battery': 1.0,
 'Lower voltage cut-off [V]': 3.105,
 'Upper voltage cut-off [V]': 4.1,
 'Open-circuit voltage at 0% SOC [V]': 3.105,
 'Open-circuit voltage at 100% SOC [V]': 4.1,
 'Initial concentration in negative electrode [mol.m-3]': 19986.609595075,
 'Initial concentration in positive electrode [mol.m-3]': 30730.7554385565,
 'Initial temperature [K]': 298.15,
 'citations': ['Marquis2019']}

CODE with _parameter_info:--> In develop branch

{k: v for k,v in spm.default_parameter_values.items() if k in spm._parameter_info}

OUTPUT:

{'Ideal gas constant [J.K-1.mol-1]': 8.314462618,
 'Faraday constant [C.mol-1]': 96485.33212,
 'Negative electrode thickness [m]': 0.0001,
 'Separator thickness [m]': 2.5e-05,
 'Positive electrode thickness [m]': 0.0001,
 'Electrode height [m]': 0.137,
 'Electrode width [m]': 0.207,
 'Nominal cell capacity [A.h]': 0.680616,
 'Current function [A]': 0.680616,
 'Maximum concentration in negative electrode [mol.m-3]': 24983.2619938437,
 'Negative electrode diffusivity [m2.s-1]': <function pybamm.input.parameters.lithium_ion.Marquis2019.graphite_mcmb2528_diffusivity_Dualfoil1998(sto, T)>,
 'Negative electrode OCP [V]': <function pybamm.input.parameters.lithium_ion.Marquis2019.graphite_mcmb2528_ocp_Dualfoil1998(sto)>,
 'Negative electrode porosity': 0.3,
 'Negative electrode active material volume fraction': 0.6,
 'Negative particle radius [m]': 1e-05,
 'Negative electrode Bruggeman coefficient (electrolyte)': 1.5,
 'Negative electrode Bruggeman coefficient (electrode)': 1.5,
 'Negative electrode exchange-current density [A.m-2]': <function pybamm.input.parameters.lithium_ion.Marquis2019.graphite_electrolyte_exchange_current_density_Dualfoil1998(c_e, c_s_surf, c_s_max, T)>,
 'Negative electrode OCP entropic change [V.K-1]': <function pybamm.input.parameters.lithium_ion.Marquis2019.graphite_entropic_change_Moura2016(sto, c_s_max)>,
 'Maximum concentration in positive electrode [mol.m-3]': 51217.9257309275,
 'Positive electrode diffusivity [m2.s-1]': <function pybamm.input.parameters.lithium_ion.Marquis2019.lico2_diffusivity_Dualfoil1998(sto, T)>,
 'Positive electrode OCP [V]': <function pybamm.input.parameters.lithium_ion.Marquis2019.lico2_ocp_Dualfoil1998(sto)>,
 'Positive electrode porosity': 0.3,
 'Positive electrode active material volume fraction': 0.5,
 'Positive particle radius [m]': 1e-05,
 'Positive electrode Bruggeman coefficient (electrolyte)': 1.5,
 'Positive electrode Bruggeman coefficient (electrode)': 1.5,
 'Positive electrode exchange-current density [A.m-2]': <function pybamm.input.parameters.lithium_ion.Marquis2019.lico2_electrolyte_exchange_current_density_Dualfoil1998(c_e, c_s_surf, c_s_max, T)>,
 'Positive electrode OCP entropic change [V.K-1]': <function pybamm.input.parameters.lithium_ion.Marquis2019.lico2_entropic_change_Moura2016(sto, c_s_max)>,
 'Separator porosity': 1.0,
 'Separator Bruggeman coefficient (electrolyte)': 1.5,
 'Initial concentration in electrolyte [mol.m-3]': 1000.0,
 'Reference temperature [K]': 298.15,
 'Ambient temperature [K]': 298.15,
 'Number of electrodes connected in parallel to make a cell': 1.0,
 'Number of cells connected in series to make a battery': 1.0,
 'Lower voltage cut-off [V]': 3.105,
 'Upper voltage cut-off [V]': 4.1,
 'Initial concentration in negative electrode [mol.m-3]': 19986.609595075,
 'Initial concentration in positive electrode [mol.m-3]': 30730.7554385565}

Clear difference can be observed between the two outputs.

NOTE: I used the output from develop branch for the second code, because in the current branch, we did not use ._parameter_info at all, so it has the same value with which it was initialised with, i.e. None

So the first solution that comes to mind is that we replace ._parameter_info with .get_parameter_info by modifying the dict. comprehension and cross check the parameters which are needed or not.

B) Another problem is, the current get_parameter_info returns a list of tuples and not a dictionary.
So, the following two are the proposed solutions.

The roundabout way:
We can fix this by just changing the dictionary comprehension to check for name attributes in the list of tuples, returned by get_parameter_info

We can achieve this by using the following code snippet

{k: v for k, v in spm.default_parameter_values.items() if any(k == getattr(param, 'name', str(param)) for param, _ in spm.get_parameter_info())}

This snippet gives the same output we get when we use _parameter_info from develop branch. This snippet can be used for the cells below with the same problem, with a small modification.

The "root" way:
Another way to solve this issue is by rebuilding the entire get_parameter_info and print_parameter_info methods to use dictionaries. This was actually suggested by @tinosulzer

which returns a list (instead of printing) - or maybe a dictionary

This would make things easier for future development and contributors, for it would make things consistent and easy to understand at a glance.

--> So, I think it would be better if we could rebuild the methods to use dictionaries instead of list of tuples.

I am open to discussions and ideas, please do comment anything that comes to your mind. In the meantime, I am rebuilding the methods to use dictionaries.

Thanks!

… print parameter info from a dictionary. Modified "print_parameter_info" to store the info in a list "table" and then print it.

…er_info-functionality' into issue-3361-improve-print_parameter_info-functionality # Conflicts: # pybamm/models/base_model.py

… print parameter info from a dictionary. Modified "print_parameter_info" to store the info in a list "table" and then print it.

…and corrected method mentioned in "1.9. Finding the parameters in a model" in "parameterization.ipynb" notebook

review-notebook-app · 2023-12-10T12:20:16Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

…unctionality

agriyakhetarpal · 2023-12-11T01:20:40Z

Ah I probably misphrased my comment. Yes, the cell's output contained identical parameters with or without _parameter_info as you mentioned in point A but we shouldn't provide those extra parameters (use default_parameter_values but parse it using get_parameter_info). The code snippet in B.1 is a bit excessive and definitely makes things unreadable. I agree with B.2 – returning a dictionary means we can access elements by the key instead of having to look at the index when accessing elements in the list of tuples. I was initially going to suggest the same as a new issue when reviewing the changes earlier, thanks for taking that up further in the same PR!

cringeyburger · 2023-12-11T09:15:16Z

I think we are ready for testing. Hoping it'll pass all tests so we can merge this branch without any issues.

agriyakhetarpal

Thanks, @cringeyburger! Looks good to me now – just a few comments on style and docstrings

CHANGELOG.md

pybamm/models/base_model.py

cringeyburger · 2023-12-12T05:15:52Z

Thanks for the help!

Also, I noticed that in the previous commit, the PR failed again on benchmark tests (well, the good thing is it passed all other tests).
It was something related to regressions and Casadi solvers, and it failed two cases (sorry, I don't have a good idea about what and how they work). I am just curious to learn.
So, what is the issue regarding this? The PR also failed the last time.

agriyakhetarpal · 2023-12-12T07:56:24Z

In the previous commit, the PR failed again on benchmark tests (well, the good thing is it passed all other tests).
It was something related to regressions and Casadi solvers, and it failed two cases (sorry, I don't have a good idea about what and how they work). I am just curious to learn.
So, what is the issue regarding this? The PR also failed the last time.

This is an issue with GitHub Actions runners in general – they run on indefinite infrastructure and are stochastic with their specifications, which can sometimes mean slower benchmark times and therefore failures. The regressions appear when the time taken for the benchmarks' code to run is longer than a threshold we have set for those benchmarks, so having them unreliably occur once in a blue moon or twice for that matter is alright; it should not be a large slowdown (say, of the order of 3x–4x) . I don't think your changes in this PR are touching any parts of the benchmarks so these were just statistical outliers.

cringeyburger · 2023-12-12T08:25:38Z

Thanks! That was informative. So, we won't face any problems with merging this PR, right?

cringeyburger · 2023-12-14T09:36:37Z

Hey @agriyakhetarpal @tinosulzer! This PR is ready for testing. I would appreciate it if we proceed further.

agriyakhetarpal · 2023-12-14T10:46:40Z

Yes, this should be ready and safe to merge. The Linux and macOS failures are just a one-off CMake cache issue which came from an update to its version – the cache contained 3.27 but 3.28 ended up being used in this case (probably a new release upstream). I shall fix that in a different PR.

cringeyburger added 3 commits November 29, 2023 23:47

Added "parameter_info" and modified "print_parameter_info"

562815b

Added docstrings and exception handling to parameter_info and print_p…

d9a1d94

…arameter_info

Added Issue 3361 to features' section of CHANGELOG.md

b86f58c

cringeyburger mentioned this pull request Dec 3, 2023

Print parameter info by submodel #1500

Closed

valentinsulzer requested changes Dec 5, 2023

View reviewed changes

pybamm/models/base_model.py Outdated Show resolved Hide resolved

pybamm/models/base_model.py Outdated Show resolved Hide resolved

pybamm/models/base_model.py Outdated Show resolved Hide resolved

Implemented "get_parameter_info" to return a dictionary of parameters…

e2f8a43

… and modified "print_parameter_info" to print the dictionary

Improved readability

d38a8b5

valentinsulzer reviewed Dec 5, 2023

View reviewed changes

pybamm/models/base_model.py Outdated Show resolved Hide resolved

agriyakhetarpal reviewed Dec 5, 2023

View reviewed changes

pybamm/models/base_model.py Show resolved Hide resolved

pybamm/models/base_model.py Show resolved Hide resolved

brosaplanella reviewed Dec 6, 2023

View reviewed changes

pybamm/models/base_model.py Outdated Show resolved Hide resolved

cringeyburger added 2 commits December 7, 2023 00:07

Added docstrings and modified "get_parameter_info" to store the entir…

6bcc50c

…e parameters instead of their names

Implemented a parameter table to print parameter info, from"print_par…

5fb4ed2

…ameter_info" (initial version)

parameter table size optimisation and code formatting

01723a5

agriyakhetarpal reviewed Dec 7, 2023

View reviewed changes

pybamm/models/base_model.py Outdated Show resolved Hide resolved

pybamm/models/base_model.py Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

cringeyburger added 2 commits December 7, 2023 18:14

Made the parameter table dynamic and modified docstrings of "get_para…

292086d

…meter_info" and "print_parameter_info"

Added "get_parameter_info" and modified information about "print_para…

3ab36c0

…meter_info" in CHANGELOG.md

agriyakhetarpal approved these changes Dec 7, 2023

View reviewed changes

Merge branch 'develop' into issue-3361-improve-print_parameter_info-f…

9c46caf

…unctionality

valentinsulzer reviewed Dec 7, 2023

View reviewed changes

pybamm/models/base_model.py Outdated Show resolved Hide resolved

Update pybamm/models/base_model.py

0da48b2

valentinsulzer approved these changes Dec 7, 2023

View reviewed changes

cringeyburger added 4 commits December 10, 2023 17:30

Modified "get_parameter_info" and "print_parameter_info" to store and…

af26526

… print parameter info from a dictionary. Modified "print_parameter_info" to store the info in a list "table" and then print it.

Merge remote-tracking branch 'origin/issue-3361-improve-print_paramet…

1618199

…er_info-functionality' into issue-3361-improve-print_parameter_info-functionality # Conflicts: # pybamm/models/base_model.py

Modified "get_parameter_info" and "print_parameter_info" to store and…

5ccd185

… print parameter info from a dictionary. Modified "print_parameter_info" to store the info in a list "table" and then print it.

Updated all instances of "_parameter_info" with "get_parameter_info" …

d365023

…and corrected method mentioned in "1.9. Finding the parameters in a model" in "parameterization.ipynb" notebook

Merge branch 'develop' into issue-3361-improve-print_parameter_info-f…

53bedfd

…unctionality

agriyakhetarpal approved these changes Dec 11, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pybamm/models/base_model.py Outdated Show resolved Hide resolved

pybamm/models/base_model.py Outdated Show resolved Hide resolved

Style and docstring modifications

55a70da

valentinsulzer approved these changes Dec 16, 2023

View reviewed changes

valentinsulzer merged commit 256747c into pybamm-team:develop Dec 16, 2023
34 of 35 checks passed

agriyakhetarpal mentioned this pull request Jan 2, 2024

Improve documentation (ongoing) #3392

Open

46 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 3361 - Improve print_parameter_info functionality #3584

Issue 3361 - Improve print_parameter_info functionality #3584

cringeyburger commented Dec 3, 2023 •

edited

Loading

cringeyburger commented Dec 3, 2023

cringeyburger commented Dec 3, 2023

codecov bot commented Dec 4, 2023 •

edited

Loading

valentinsulzer left a comment

cringeyburger commented Dec 5, 2023

valentinsulzer left a comment

agriyakhetarpal left a comment

brosaplanella left a comment

cringeyburger commented Dec 7, 2023

cringeyburger commented Dec 7, 2023

agriyakhetarpal left a comment

cringeyburger commented Dec 7, 2023

valentinsulzer left a comment

cringeyburger commented Dec 7, 2023

valentinsulzer commented Dec 7, 2023

agriyakhetarpal commented Dec 8, 2023

cringeyburger commented Dec 8, 2023

agriyakhetarpal commented Dec 9, 2023 •

edited

Loading

cringeyburger commented Dec 10, 2023 •

edited

Loading

review-notebook-app bot commented Dec 10, 2023

agriyakhetarpal commented Dec 11, 2023

cringeyburger commented Dec 11, 2023

agriyakhetarpal left a comment

cringeyburger commented Dec 12, 2023 •

edited

Loading

agriyakhetarpal commented Dec 12, 2023

cringeyburger commented Dec 12, 2023

cringeyburger commented Dec 14, 2023

agriyakhetarpal commented Dec 14, 2023

Issue 3361 - Improve print_parameter_info functionality #3584

Issue 3361 - Improve print_parameter_info functionality #3584

Conversation

cringeyburger commented Dec 3, 2023 • edited Loading

Description

Type of change

Key checklist:

Further checks:

cringeyburger commented Dec 3, 2023

cringeyburger commented Dec 3, 2023

codecov bot commented Dec 4, 2023 • edited Loading

Codecov Report

valentinsulzer left a comment

Choose a reason for hiding this comment

cringeyburger commented Dec 5, 2023

valentinsulzer left a comment

Choose a reason for hiding this comment

agriyakhetarpal left a comment

Choose a reason for hiding this comment

brosaplanella left a comment

Choose a reason for hiding this comment

cringeyburger commented Dec 7, 2023

cringeyburger commented Dec 7, 2023

agriyakhetarpal left a comment

Choose a reason for hiding this comment

cringeyburger commented Dec 7, 2023

valentinsulzer left a comment

Choose a reason for hiding this comment

cringeyburger commented Dec 7, 2023

valentinsulzer commented Dec 7, 2023

agriyakhetarpal commented Dec 8, 2023

cringeyburger commented Dec 8, 2023

agriyakhetarpal commented Dec 9, 2023 • edited Loading

cringeyburger commented Dec 10, 2023 • edited Loading

review-notebook-app bot commented Dec 10, 2023

agriyakhetarpal commented Dec 11, 2023

cringeyburger commented Dec 11, 2023

agriyakhetarpal left a comment

Choose a reason for hiding this comment

cringeyburger commented Dec 12, 2023 • edited Loading

agriyakhetarpal commented Dec 12, 2023

cringeyburger commented Dec 12, 2023

cringeyburger commented Dec 14, 2023

agriyakhetarpal commented Dec 14, 2023

cringeyburger commented Dec 3, 2023 •

edited

Loading

codecov bot commented Dec 4, 2023 •

edited

Loading

agriyakhetarpal commented Dec 9, 2023 •

edited

Loading

cringeyburger commented Dec 10, 2023 •

edited

Loading

cringeyburger commented Dec 12, 2023 •

edited

Loading