Issue 2334 reformat parameters #2342

valentinsulzer · 2022-10-04T18:26:48Z

Description

Change parameters to python files (+ some csv files). Breaking change.

Fixes #2334
Fixes #2077

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

review-notebook-app · 2022-10-05T22:53:00Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2022-10-06T18:45:56Z

Codecov Report

Base: 99.65% // Head: 99.65% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (408db86) compared to base (cb8a757).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 408db86 differs from pull request most recent head c3b0676. Consider uploading reports for the commit c3b0676 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##           develop    #2342    +/-   ##
=========================================
  Coverage    99.65%   99.65%            
=========================================
  Files          361      269    -92     
  Lines        19948    20079   +131     
=========================================
+ Hits         19879    20010   +131     
  Misses          69       69

Impacted Files	Coverage Δ
pybamm/input/parameters/lead_acid/Sulzer2019.py	`100.00% <100.00%> (ø)`
pybamm/input/parameters/lithium_ion/Ai2020.py	`100.00% <100.00%> (ø)`
pybamm/input/parameters/lithium_ion/Chen2020.py	`100.00% <100.00%> (ø)`
...input/parameters/lithium_ion/Chen2020_composite.py	`100.00% <100.00%> (ø)`
pybamm/input/parameters/lithium_ion/Ecker2015.py	`100.00% <100.00%> (ø)`
pybamm/input/parameters/lithium_ion/Marquis2019.py	`100.00% <100.00%> (ø)`
pybamm/input/parameters/lithium_ion/Mohtat2020.py	`100.00% <100.00%> (ø)`
pybamm/input/parameters/lithium_ion/NCA_Kim2011.py	`100.00% <100.00%> (ø)`
pybamm/input/parameters/lithium_ion/OKane2022.py	`100.00% <100.00%> (ø)`
pybamm/input/parameters/lithium_ion/ORegan2022.py	`100.00% <100.00%> (ø)`
... and 71 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rtimms

thanks, this is much simpler!

pybamm/input/parameters/lithium_ion/Ai2020.py

rtimms · 2022-10-10T12:41:31Z

pybamm/input/parameters/lithium_ion/Ai2020.py

+    """
+    m_ref = (
+        1 * 10 ** (-11) * pybamm.constants.F
+    )  # (A/m2)(mol/m3)**1.5 - includes ref concentrations


I think this comment is wrong everywhere and has been forever haha. Shouldn't the units be (A/m2)(m3/mol)**1.5 or equivalent?

I agree with Rob. I think it makes sense to leave the units, corrected though, as a quick sanity check.

rtimms · 2022-10-10T12:49:19Z

pybamm/parameters/parameter_values.py

+        output = preamble + "\n\n" + output
+
+        # Add pybamm. to functions that didn't have it in function body before
+        for funcname in [


should we add a broader set of "standard" functions here or wait and add them until people need them?

not sure ... also not sure whether we really need this function at all tbh?

brosaplanella

Amazing! Thanks @tinosulzer :)

pybamm/input/parameters/lithium_ion/Ai2020.py

brosaplanella · 2022-10-10T13:01:04Z

pybamm/input/parameters/lithium_ion/Ai2020.py

+    """
+    m_ref = (
+        1 * 10 ** (-11) * pybamm.constants.F
+    )  # (A/m2)(mol/m3)**1.5 - includes ref concentrations


I agree with Rob. I think it makes sense to leave the units, corrected though, as a quick sanity check.

brosaplanella · 2022-10-10T13:11:56Z

tests/unit/test_parameters/test_parameter_sets/test_LCO_Ai2020.py

+        parameter_values = pybamm.ParameterValues(
+            {
+                "chemistry": "lithium_ion",
+                "cell": "Enertech_Ai2020",
+                "negative electrode": "graphite_Ai2020",
+                "separator": "separator_Ai2020",
+                "positive electrode": "lico2_Ai2020",
+                "electrolyte": "lipf6_Enertech_Ai2020",
+                "experiment": "1C_discharge_from_full_Ai2020",
+                "sei": "example",
+                "citation": "Ai2019",
+            }
+        )


Is this needed?

brosaplanella · 2022-10-10T13:13:07Z

tests/unit/test_parameters/test_parameter_sets/test_Landesfeind2019.py

@@ -35,7 +35,7 @@ def test_load_params(self):
        for solvent in ["EC_DMC_1_1", "EC_EMC_3_7", "EMC_FEC_19_1"]:
            root = pybamm.root_dir()
            p = (
-                "pybamm/input/parameters/lithium_ion/electrolytes/lipf6_"
+                "pybamm/input/parameters/lithium_ion/testing_only/electrolytes/lipf6_"


Maybe related to the previous comment, but why is testing_only needed?

valentinsulzer · 2022-10-10T15:53:11Z

@brosaplanella I kept the testing_only parameters and tests so that it still works with the old split structure in case people are still using that. I don't know if anyone is, nobody raised any complaints about this change. Do you think I should just remove it?

valentinsulzer · 2022-10-10T16:00:11Z

pybamm/input/parameters/lithium_ion/Ai2020.py

+# Load data in the appropriate format
+path, _ = os.path.split(os.path.abspath(__file__))
+graphite_ocp_Enertech_Ai2020 = pybamm.parameters.process_1D_data(
+    "graphite_ocp_Enertech_Ai2020.csv", path=path
+)
+lico2_ocp_Ai2020 = pybamm.parameters.process_1D_data("lico2_ocp_Ai2020.csv", path=path)
+


@martinjrobins what do you think of this approach?

brosaplanella · 2022-10-10T16:37:41Z

@brosaplanella I kept the testing_only parameters and tests so that it still works with the old split structure in case people are still using that. I don't know if anyone is, nobody raised any complaints about this change. Do you think I should just remove it?

Makes sense, I agree we should keep it at least for a while.

valentinsulzer · 2022-10-11T18:10:48Z

Merging since all tests pass. We can refactor where we put the "data" parameters if needed

valentinsulzer added 5 commits September 30, 2022 15:13

add script to convert csv parameters to python file

251b85a

#2334 working on loading new-style parameters

4d8c229

#2334 make printing python a method of ParameterValues

69324c5

add readmes and better parameter files to generated python scripts

e96ec5d

#2334 fix load_function

ff344a8

valentinsulzer marked this pull request as draft October 4, 2022 18:26

valentinsulzer and others added 2 commits October 4, 2022 16:46

#2334 fixing tests

0d56ecf

style: pre-commit fixes

2a75a8d

valentinsulzer mentioned this pull request Oct 4, 2022

Add action to format code with black #2272

Merged

8 tasks

valentinsulzer added 9 commits October 4, 2022 17:17

#2334 more test fixes

7f05564

merge

4e9469a

#2334 tests

4b13ab0

trying to fix coverage issue

aa5d84e

#2334 remove old parameters and clean up readmes

62eb4b3

#2334 fixing tests

e40e21d

Merge branch 'develop' into issue-2334-reformat-parameters

1bf5896

#2334 tests

a05ac21

#2334 still testing

25a12ce

valentinsulzer added 8 commits October 5, 2022 19:03

#2334 fix parameter_values tests

ab551d4

#2334 fix landesfeind

090cc1f

#2334 integration tests

fbe4a2b

#2334 working on example tests, undelete update_parameter_sets_doc.py

8c2d561

#2334 fix test, try full path in codecov.yml

f7ca03a

#2334 changelog

60bdd60

move generated parameter file outside of pybamm dir

5051cd4

flake8

f73ddd5

valentinsulzer added 2 commits October 6, 2022 16:36

#2334 coverage

7abfe84

#2334 flake8

a526810

valentinsulzer added 4 commits October 6, 2022 17:26

#2334 fix duplicated name

1fbf2a9

#2334 coverage

778a8d5

#2334 fix lead-acid tests

f014848

#2334 fix example

408db86

valentinsulzer marked this pull request as ready for review October 7, 2022 03:19

valentinsulzer requested review from brosaplanella and rtimms October 7, 2022 03:19

rtimms approved these changes Oct 10, 2022

View reviewed changes

brosaplanella approved these changes Oct 10, 2022

View reviewed changes

merge develop

c2b6537

valentinsulzer commented Oct 10, 2022

View reviewed changes

#2334 ferran and rob comments

c3b0676

valentinsulzer merged commit 924ee9e into develop Oct 11, 2022

valentinsulzer deleted the issue-2334-reformat-parameters branch October 11, 2022 18:10

awadell1 mentioned this pull request Oct 24, 2022

Discover Parameter Sets using Entry Points #2396

Merged

8 tasks

rtimms mentioned this pull request Nov 1, 2022

Update parameter management notebook #1541

Closed

Saransh-cpp mentioned this pull request Nov 1, 2022

Migrate to PyBaMM 22.10 pybamm-team/BattBot#105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 2334 reformat parameters #2342

Issue 2334 reformat parameters #2342

valentinsulzer commented Oct 4, 2022 •

edited

Loading

review-notebook-app bot commented Oct 5, 2022

codecov bot commented Oct 6, 2022 •

edited

Loading

rtimms left a comment

rtimms Oct 10, 2022

brosaplanella Oct 10, 2022

rtimms Oct 10, 2022

valentinsulzer Oct 10, 2022

brosaplanella left a comment

brosaplanella Oct 10, 2022

brosaplanella Oct 10, 2022

brosaplanella Oct 10, 2022

valentinsulzer commented Oct 10, 2022

valentinsulzer Oct 10, 2022

brosaplanella commented Oct 10, 2022

valentinsulzer commented Oct 11, 2022

Issue 2334 reformat parameters #2342

Issue 2334 reformat parameters #2342

Conversation

valentinsulzer commented Oct 4, 2022 • edited Loading

Description

Type of change

Key checklist:

Further checks:

review-notebook-app bot commented Oct 5, 2022

codecov bot commented Oct 6, 2022 • edited Loading

Codecov Report

rtimms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brosaplanella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinsulzer commented Oct 10, 2022

Choose a reason for hiding this comment

brosaplanella commented Oct 10, 2022

valentinsulzer commented Oct 11, 2022

valentinsulzer commented Oct 4, 2022 •

edited

Loading

codecov bot commented Oct 6, 2022 •

edited

Loading