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.

Yep! This is not editing things in-place.

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.

CHANGELOG.md

src/pybamm/models/full_battery_models/base_battery_model.py

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?

Ah, I think what happened was that @prady0t and I added in our reviews at the same time, and I didn't see his review (sorry!)

Yes, handling numeric or boolean options is needed – please feel free to ignore my suggestion in this case. Thanks!

src/pybamm/models/full_battery_models/base_battery_model.py

Saransh-cpp · 2024-12-24T18:51:53Z

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"])


Yep! This is not editing things in-place.

Saransh-cpp · 2024-12-24T18:57:56Z

src/pybamm/models/full_battery_models/base_battery_model.py

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


Are there cases where value can be something other than str or tuple? If yes, then the type signature of this function is wrong. If not, None should not be returned. (It is always preferred to not have the return value of a function depend on an if condition, so removing the None would be the ideal case.)

Also, I would really prefer a nested if-else block here for readability.

Ah, just saw @agriyakhetarpal's comment above.

The type signature should still be fixed + it should be converted into an if-else block.

medha-14 · 2024-12-25T13:05:55Z

Would it be wrong to assume that all options passed can be expressed in two ways:

If a single value is passed for an option, it would look like this:

options = {
    "working electrode": "both",
}

If multiple values are passed, it would be structured like this:

options = {
    "SEI": ("none", "constant"),
}

Are these the only two possibilities?
or should we consider some different input formats as well?

rtimms · 2025-01-02T11:39:05Z

@medha-14 yes, those are the only two options. I think what I had in mind here is that all the options that can be tuples should be converted to tuples in-place at the start, then we can always just treat them like they will be tuples. You could add a new constant that is a list of all the options that can be tuples.

medha-14 · 2025-01-12T07:55:11Z

Is it essential to convert the options to tuples beforehand? In my recent commit, I addressed the issue by updating:

if options["particle size"] == "distribution"

to this

if "distribution" in options["particle size"] :

Like in the commit i made
This approach works for both string and tuple options. Please let me know if I am overlooking anything.

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

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

prady0t reviewed Dec 23, 2024

View reviewed changes

agriyakhetarpal reviewed Dec 23, 2024

View reviewed changes

Saransh-cpp requested changes Dec 24, 2024

View reviewed changes

medha-14 added 6 commits January 12, 2025 11:37

improved option checking

ae0cf4e

added changelog

8640332

minor fixes

c4d027b

formatting

020a74d

minor changes

68cd6d7

remove ensure tuple approach

08732b9

medha-14 force-pushed the optioncheck branch from a124331 to 08732b9 Compare January 12, 2025 06:45

medha-14 and others added 2 commits January 12, 2025 12:19

Merge branch 'develop' into optioncheck

6cc1bfd

style: pre-commit fixes

e52e792

Merge branch 'develop' into optioncheck

3a5f24c

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

Saransh-cpp Dec 24, 2024

prady0t Dec 23, 2024 •

edited

Loading

agriyakhetarpal left a comment

agriyakhetarpal Dec 23, 2024

medha-14 Dec 24, 2024 •

edited

Loading

agriyakhetarpal Dec 24, 2024

Saransh-cpp Dec 24, 2024

Saransh-cpp Dec 24, 2024

Saransh-cpp Dec 24, 2024

Saransh-cpp Dec 24, 2024

medha-14 commented Dec 25, 2024

rtimms commented Jan 2, 2025

medha-14 commented Jan 12, 2025 •

edited

Loading

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

Saransh-cpp Dec 24, 2024

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

medha-14 Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

agriyakhetarpal Dec 24, 2024

Choose a reason for hiding this comment

Saransh-cpp Dec 24, 2024

Choose a reason for hiding this comment

Saransh-cpp Dec 24, 2024

Choose a reason for hiding this comment

Saransh-cpp Dec 24, 2024

Choose a reason for hiding this comment

Saransh-cpp Dec 24, 2024

Choose a reason for hiding this comment

medha-14 commented Dec 25, 2024

rtimms commented Jan 2, 2025

medha-14 commented Jan 12, 2025 • edited Loading

prady0t Dec 23, 2024 •

edited

Loading

prady0t Dec 23, 2024 •

edited

Loading

prady0t Dec 23, 2024 •

edited

Loading

prady0t Dec 23, 2024 •

edited

Loading

medha-14 Dec 24, 2024 •

edited

Loading

medha-14 commented Jan 12, 2025 •

edited

Loading