Improved option checking for tuples #4707

medha-14 · 2024-12-23T07:27:58Z

Description

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 -m pytest (or $ nox -s tests)
The documentation builds: $ python -m pytest --doctest-plus src (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ 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

medha-14 · 2024-12-23T07:30:43Z

I have implemented better option checking by creating an ensure_tuple method. This method automatically converts any string-based options into tuples, ensuring consistent handling of options.

agriyakhetarpal

Thanks for starting this!

src/pybamm/models/full_battery_models/base_battery_model.py

codecov · 2024-12-23T11:03:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.22%. Comparing base (a7253b8) to head (97aa955).
Report is 5 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4707   +/-   ##
========================================
  Coverage    99.22%   99.22%           
========================================
  Files          303      303           
  Lines        23070    23103   +33     
========================================
+ Hits         22891    22924   +33     
  Misses         179      179

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

prady0t · 2024-12-23T19:57:16Z

src/pybamm/models/full_battery_models/base_battery_model.py

+        _ensure_tuple(options["open-circuit potential"])
+        _ensure_tuple(options["particle"])
+        _ensure_tuple(options["intercalation kinetics"])


Suggested change

_ensure_tuple(options["open-circuit potential"])

_ensure_tuple(options["particle"])

_ensure_tuple(options["intercalation kinetics"])

options["open-circuit potential"] = _ensure_tuple(options["open-circuit potential"])

options["particle"] = _ensure_tuple(options["particle"])

options["intercalation kinetics"] = _ensure_tuple(options["intercalation kinetics"])

@agriyakhetarpal I might be wrong but shouldn't we do these changes instead everywhere? With _ensure_tuple(options["open-circuit potential"]) we are not really changing anything?

If true we must then rename the helper function to something like _make_tuple instead.

prady0t · 2024-12-23T20:01:36Z

src/pybamm/models/full_battery_models/base_battery_model.py

@@ -653,6 +699,7 @@ def __init__(self, extra_options):

        # Check options are valid
        for option, value in options.items():
+            _ensure_tuple(value)
            if isinstance(value, str) or option in [


Shouldn't we check for

if isinstance(value, tuple)

instead?

agriyakhetarpal

This looks good to me functionality-wise, thanks. I have a few more suggestions. I'll let @rtimms advise on any other changes, since the issue was originally his idea.

agriyakhetarpal · 2024-12-23T19:46:56Z

CHANGELOG.md

@@ -156,6 +156,7 @@ package to install PyBaMM with only the required dependencies. ([conda-forge/pyb
 ## Optimizations

 - Sped up initialization of a `ProcessedVariable` by making the internal `xarray.DataArray` initialization lazy (only gets created if interpolation is needed) ([#3862](https://github.com/pybamm-team/PyBaMM/pull/3862))
+- Enhanced option checking by converting string-based options into tuples for consistent handling.([#4707](https://github.com/pybamm-team/PyBaMM/pull/4707))


This needs to be moved to the unreleased section. Also, please note the descending order in PR numbers, thanks!

Suggested change

- Enhanced option checking by converting string-based options into tuples for consistent handling.([#4707](https://github.com/pybamm-team/PyBaMM/pull/4707))

agriyakhetarpal · 2024-12-23T19:50:28Z

src/pybamm/models/full_battery_models/base_battery_model.py

@@ -18,6 +18,27 @@ def represents_positive_integer(s):
        return val > 0


+def _ensure_tuple(value):


Let's help type checkers :) Also, we need a from __future__ import annotations statement at the top again.

Suggested change

def _ensure_tuple(value):

def _ensure_tuple(value: str | tuple[str]) -> tuple | None:

agriyakhetarpal · 2024-12-23T20:02:57Z

src/pybamm/models/full_battery_models/base_battery_model.py

+    if isinstance(value, str):
+        return (value,)
+    elif isinstance(value, tuple):
+        return value


if-elif constructs should always end with an else: statement to catch unwarranted behaviour. But since we are the only users of this private method, it's probably fine here...

We could simplify this to something like (since we know the inputs and outputs and would never hit the None case):

Suggested change

if isinstance(value, str):

return (value,)

elif isinstance(value, tuple):

return value

return (value,) if isinstance(value, str) else value if isinstance(value, tuple) else None

Would returning none be the best approach as , there can be cases where the values can be numeric or boolean, should those cases be handled as they come.
Like the part @prady0t pointed out

for option, value in options.items(): _ensure_tuple(value) if isinstance(value, str) or option in [ "dimensionality", "operating mode", ]: # some options accept non-strings value = (value,)

Should i remove ensure_tuple here and handle such cases seperately?

agriyakhetarpal · 2024-12-23T20:03:55Z

src/pybamm/models/full_battery_models/base_battery_model.py

+        for option in [
+            "particle size",
+            "lithium plating porosity change",
+            "heat of mixing",
+            "particle",
+            "particle mechanics",
+            "particle shape",
+            "SEI",
+            "stress-induced diffusion",
+            "thermal",
+        ]:
+            _ensure_tuple(options[option])


Suggested change

for option in [

"particle size",

"lithium plating porosity change",

"heat of mixing",

"particle",

"particle mechanics",

"particle shape",

"SEI",

"stress-induced diffusion",

"thermal",

]:

_ensure_tuple(options[option])

for _ in [

"particle size",

"lithium plating porosity change",

"heat of mixing",

"particle",

"particle mechanics",

"particle shape",

"SEI",

"stress-induced diffusion",

"thermal",

]:

_ensure_tuple(options[_])

As a convention, for variables that are not needed later in the code and are used only during the context of loops, we can use _ to define them.

improved option checking

db88429

medha-14 requested review from brosaplanella, DrSOKane, rtimms, valentinsulzer and TomTranter as code owners December 23, 2024 07:27

medha-14 added 2 commits December 23, 2024 13:27

added changelog

8d65702

minor fixes

97aa955

agriyakhetarpal reviewed Dec 23, 2024

View reviewed changes

src/pybamm/models/full_battery_models/base_battery_model.py Outdated Show resolved Hide resolved

src/pybamm/models/full_battery_models/base_battery_model.py Outdated Show resolved Hide resolved

src/pybamm/models/full_battery_models/base_battery_model.py Outdated Show resolved Hide resolved

medha-14 and others added 2 commits December 23, 2024 21:02

Merge branch 'develop' into optioncheck

25ead47

formatting

e3e64e6

prady0t reviewed Dec 23, 2024

View reviewed changes

agriyakhetarpal reviewed Dec 23, 2024

View reviewed changes

minor changes

7592523

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved option checking for tuples #4707

Improved option checking for tuples #4707

medha-14 commented Dec 23, 2024

medha-14 commented Dec 23, 2024

agriyakhetarpal left a comment

codecov bot commented Dec 23, 2024

prady0t Dec 23, 2024 •

edited

Loading

prady0t Dec 23, 2024 •

edited

Loading

prady0t Dec 23, 2024 •

edited

Loading

prady0t Dec 23, 2024 •

edited

Loading

agriyakhetarpal left a comment

agriyakhetarpal Dec 23, 2024

agriyakhetarpal Dec 23, 2024 •

edited

Loading

agriyakhetarpal Dec 23, 2024

medha-14 Dec 24, 2024 •

edited

Loading

agriyakhetarpal Dec 23, 2024

		@@ -18,6 +18,27 @@ def represents_positive_integer(s):
		return val > 0


		def _ensure_tuple(value):

	def _ensure_tuple(value):
	def _ensure_tuple(value: str \| tuple[str]) -> tuple \| None:

Improved option checking for tuples #4707

Are you sure you want to change the base?

Improved option checking for tuples #4707

Conversation

medha-14 commented Dec 23, 2024

Description

Type of change

Key checklist:

Further checks:

medha-14 commented Dec 23, 2024

agriyakhetarpal left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 23, 2024

Codecov Report

prady0t Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

prady0t Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

prady0t Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

prady0t Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

agriyakhetarpal left a comment

Choose a reason for hiding this comment

agriyakhetarpal Dec 23, 2024

Choose a reason for hiding this comment

agriyakhetarpal Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

agriyakhetarpal Dec 23, 2024

Choose a reason for hiding this comment

medha-14 Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

agriyakhetarpal Dec 23, 2024

Choose a reason for hiding this comment

prady0t Dec 23, 2024 •

edited

Loading

prady0t Dec 23, 2024 •

edited

Loading

prady0t Dec 23, 2024 •

edited

Loading

prady0t Dec 23, 2024 •

edited

Loading

agriyakhetarpal Dec 23, 2024 •

edited

Loading

medha-14 Dec 24, 2024 •

edited

Loading