Issue 3361 - Improve print_parameter_info functionality

[cringeyburger]

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

[cringeyburger]

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]

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

[codecov]

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

[cringeyburger]

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 ?

[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

[brosaplanella]

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

[cringeyburger]

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.

[cringeyburger]

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?

[cringeyburger]

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! 👋

[valentinsulzer]

Happy to merge once tests pass

[cringeyburger]

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]

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

[agriyakhetarpal]

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]

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

[agriyakhetarpal]

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]

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:

1. 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']}

2. 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.

1. 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.

2. 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!

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[agriyakhetarpal]

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]

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

[cringeyburger]

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]

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]

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

[cringeyburger]

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

[agriyakhetarpal]

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.