Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 563 parameters by material #647

Merged
merged 34 commits into from
Oct 14, 2019

Conversation

valentinsulzer
Copy link
Member

@valentinsulzer valentinsulzer commented Oct 7, 2019

Description

  • Reorganised parameters by material.
  • Specified path to functions in csv files
  • Allowed setting either current or C-rate
  • Updated docs on how to add new parameters

Fixes #563

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Key checklist:

  • No style issues: $ flake8
  • All tests pass: $ python run-tests.py --unit
  • The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works
  • Any dependent changes have been merged and published in downstream modules

@rtimms
Copy link
Contributor

rtimms commented Oct 7, 2019

some comments for discussion:

  • I think there could be a "geometry" or "cell" file which contains the things like cell geometry and maybe other cell-scale parameters like voltage cut-off or number of cells to make a battery. Maybe nominal cell capacity as stated by manufacturer. From this we then define the typical current to be 1C based on the nominal cell capacity.

  • For li-ion we will need a set of thermal parameters. The structure could be the same for lead-acid, with a boring file with the ref and max temp in

  • It would be good to have the ability to just pass the name of a paper to quickly set all the parameters from that paper

  • I think a misc params file is probably ok, but should also be named with author_year

  • maybe not for this issue, but for effective properties e.g. conductivity it would be nice to select how they are calculated e.g. bruggeman or otherwise. At the moment we hard-code bruggeman into the models

  • I think this has already been discussed, but where is the best place to set initial conditions?

@rtimms rtimms mentioned this pull request Oct 7, 2019
@brosaplanella
Copy link
Sponsor Member

I like @Scottmar93 suggestion in #563 about defining an experiment. It is good to distinguish between the following: battery parameters (intrinsic of the battery materials or, from a maths point of view, of the equations), which could be split into further subcategories; geometry and those related to the experiment (how we cycle the battery). This poses some interesting questions about defining the SoC of the electrodes (or maybe we should use state of lithiation to avoid confusion) as they are related to the potentials.

I agree also with @rtimms points above. In particular, I think that we should define a new parameter/function that accounts for the effectiveness of the medium, so then one can choose between different models. In particular, I think it would be interesting to use the values obtained via homogenisation using Rayleigh's multipole method (see Bruna & Chapman 2015).

@codecov
Copy link

codecov bot commented Oct 9, 2019

Codecov Report

Merging #647 into master will increase coverage by 0.03%.
The diff coverage is 99.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #647      +/-   ##
==========================================
+ Coverage   98.49%   98.53%   +0.03%     
==========================================
  Files         167      169       +2     
  Lines        8180     8255      +75     
==========================================
+ Hits         8057     8134      +77     
+ Misses        123      121       -2
Impacted Files Coverage Δ
...erface/diffusion_limited/full_diffusion_limited.py 100% <ø> (ø) ⬆️
...ollector/effective_resistance_current_collector.py 96.29% <0%> (+1.11%) ⬆️
pybamm/parameters/print_parameters.py 100% <100%> (ø) ⬆️
...l_battery_models/lead_acid/base_lead_acid_model.py 100% <100%> (ø) ⬆️
pybamm/expression_tree/simplify.py 94.98% <100%> (ø) ⬆️
...bamm/parameters/standard_parameters_lithium_ion.py 100% <100%> (ø) ⬆️
pybamm/expression_tree/functions.py 100% <100%> (ø) ⬆️
...models/submodels/electrode/ohm/surface_form_ohm.py 96.55% <100%> (ø) ⬆️
...m/full_surface_form_stefan_maxwell_conductivity.py 100% <100%> (ø) ⬆️
pybamm/parameters/standard_parameters_lead_acid.py 99.15% <100%> (+0.02%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4767123...e694737. Read the comment docs.

Copy link
Contributor

@rtimms rtimms left a comment

Choose a reason for hiding this comment

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

this looks great, thanks @tinosulzer !

"separator": "separator_Marquis2019",
"cathode": "lico2_Marquis2019",
"electrolyte": "lipf6_Marquis2019",
"experiment": "1C_discharge_from_full_Marquis2019",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe there should be a very brief description of the parameters that go into the experiment folder? Though maybe it is clear from looking at the existing parameter sets.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I also think that a list of the required parameters in the documentation would be helpful.

@@ -0,0 +1,49 @@
#
# Parameter sets from papers
Copy link
Contributor

Choose a reason for hiding this comment

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

this is very user-friendly!

@valentinsulzer valentinsulzer merged commit 9171302 into master Oct 14, 2019
@valentinsulzer valentinsulzer deleted the issue-563-parameters-by-material branch October 14, 2019 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add parameters by material
3 participants