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 718 cooling area #1022

Merged
merged 67 commits into from
May 28, 2020
Merged

Conversation

brosaplanella
Copy link
Member

Description

The lumped thermal model now takes an option to choose between arbitrary geometry (default) or a pouch geometry. The arbitrary geometry takes a total heat transfer coefficient, surface cooling area and cell volume. The pouch geometry calculates it from the different parts of the pouch (including tabs).

Fixes #718

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: $ 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

brosaplanella and others added 30 commits May 26, 2020 10:46
@brosaplanella
Copy link
Member Author

Oh yes, in that case it makes sense. Is it only for dimensionality = 2 x-lumped model? Do we want to throw an error or just a warning?

@rtimms
Copy link
Contributor

rtimms commented May 27, 2020

the x-lumped models assume a pouch cell for conditionality 1 and 2. I think it would be good to raise an option error. If people really want to to do it they can bypass the option error by manually changing submodels

@brosaplanella
Copy link
Member Author

Actually, x-lumped models of higher dimension already assume pouch (I have only implemented arbitrary geometry for lumped models). Dimensionality 0 will fall back to the lumped model I think so then the arbitrary geometry is fine.

@rtimms
Copy link
Contributor

rtimms commented May 27, 2020

Yeah it is probably fine as is. I was just worried that people might choose the options {"dimensionality": 2, "thermal": "x-lumped", "geometry": "arbitrary"} which at the moment silently works ok, but people might not realise that choosing "arbitrary" has no effect. Maybe there could be a setter for geometry that picks the default based on some other options (if it isn't provided). But perhaps I am unnecessarily worried and that is too complicated!

@rtimms rtimms merged commit eeccaa9 into pybamm-team:develop May 28, 2020
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.

alternative geometries for lumped thermal models
3 participants