Renamed 'Cell capacity [A.h]' to 'Nominal cell capacity [A.h]'

[Saransh-cpp]

Description

Addressing #1339, changed the name, added value error and deprecation error. Hopefully this closes the issue.

Type of change

Renamed 'Cell capacity [A.h]'. I will add the following changes in CHANGELOG.md and list it in the breaking changes section.

• 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

[Saransh-cpp]

Please guide me as to how to fix the failing checks.

[brosaplanella]

Looks good in general, thanks @Saransh-cpp! Ignore the macos tests for the moment as they fail for another reason. The ubuntu tests fail because there are some examples (both scripts and notebooks) which still call "Cell capacity [A.h]".

[Saransh-cpp]

Looks good in general, thanks @Saransh-cpp! Ignore the macos tests for the moment as they fail for another reason. The ubuntu tests fail because there are some examples (both scripts and notebooks) which still call "Cell capacity [A.h]".

Working on it!

[codecov]

Codecov Report

Merging #1352 (df0ab56) into develop (d90636a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1352   +/-   ##
========================================
  Coverage    98.15%   98.15%           
========================================
  Files          272      272           
  Lines        15577    15605   +28     
========================================
+ Hits         15289    15317   +28     
  Misses         288      288           

Impacted Files	Coverage Δ
pybamm/parameters/electrical_parameters.py	100.00% <100.00%> (ø)
pybamm/parameters/parameter_values.py	99.38% <100.00%> (+<0.01%)	⬆️
pybamm/simulation.py	97.15% <100.00%> (ø)
...m/models/full_battery_models/base_battery_model.py	99.67% <0.00%> (ø)
...dels/electrolyte_conductivity/full_conductivity.py	100.00% <0.00%> (ø)
...e_potential_form/full_surface_form_conductivity.py	100.00% <0.00%> (ø)
pybamm/spatial_methods/spectral_volume.py	98.11% <0.00%> (+0.01%)	⬆️
...lyte_conductivity/base_electrolyte_conductivity.py	98.83% <0.00%> (+0.35%)	⬆️

---

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 d90636a...df0ab56. Read the comment docs.

[brosaplanella]

The last thing to do is to add a test to improve the coverage (see the Codecov report above). The error and warning messages in parameter_values.py are not tested and that's why the coverage fails. To fix it, you have to add some new tests in test_parameter_values.py. You can add them in the function test_check_parameter_values (line 95). Note that you will have to test both the error and the warning. The warning can be tested doing self.assertWarnsRegex(DeprecationWarning, "some words from your warning message here").

[Saransh-cpp]

self.assertWarnsRegex(DeprecationWarning, "some words from your warning message here")

with self.assertRaisesRegex(ValueError, "Cell capacity"):
            pass1
with self.assertWarnsRegex(DeprecationWarning, "Cell capacity [A.h]"):
            pass2

I was trying to write the tests but cannot figure out what should I replace 'pass1' and 'pass2' with?

I tried writing pybamm.ParameterValues(values["Nominal cell capacity [A.h]"] = values["Cell capacity [A.h]"]) at pass1 but it shows me an error, then I tried to use the values= field but I don't know what I should equate it with, should I just write pybamm.ParameterValues({"Cell capacity [A.h]": 0})?

For pass2, I saw a similar test at line 665 of the same file but again I can't figure out what to equate values= with (as I won't be using chemistry= here).

Please guide,
Thanks.

[brosaplanella]

So, these lines should contain a command that you expect to trigger the error/warning. For the warning, I think that pybamm.ParameterValues({"Cell capacity [A.h]": 0}) will do. For the error, I think you need pybamm.ParameterValues({"Cell capacity [A.h]": 0, "Nominal cell capacity [A.h]": 0}).

[Saransh-cpp]

Thank you so much @brosaplanella

[brosaplanella]

You need to pass DeprecationWarning as an argument (see below) otherwise it raises a generic warning and that's why the test fails.

                warnings.warn(
                    "the 'Cell capacity [A.h]' notation will be "
                    "deprecated in the next release, as it has now been renamed "
                    "to 'Nominal cell capacity [A.h]'. Simulation will continue "
                    "passing the 'Cell capacity [A.h]' as 'Nominal cell "
                    "capacity [A.h]' (it might overwrite any existing definition "
                    "of the component)",
                    DeprecationWarning,
                )

[Saransh-cpp]

Yes, my bad, I didn't run the tests on my side. Running them now for confirmation.

[Saransh-cpp]

After adding this and running the tests, it gives me an error

pybamm.ParameterValues({"Cell capacity [A.h]": 0})
AssertionError: "Cell capacity [A.h]" does not match "the 'Cell capacity [A.h]' notation will be deprecated in the next release, as it has now been renamed to 'Nominal cell capacity [A.h]'. Simulation 
will continue passing the 'Cell capacity [A.h]' as 'Nominal cell capacity [A.h]' (it might overwrite any existing definition of the component)"

How can I resolve this?

[brosaplanella]

I don't know... Maybe try passing only "Cell capacity". Also, I don't think it is the source of any issue, but might be good to define the capacities in the test to be 1 rather than 0, as a 0 capacity could potentially lead to further issues.

[Saransh-cpp]

I tried that but then it shows the "Deprecation warning not triggered" error. I am looking into it, if you find the issue please let me know. Thanks

[Saransh-cpp]

Yes I did that, it does not give me the "Deprecation warning not triggered" with "Cell capacity [A.h]" but it does give me the same error with "Cell capacity".

[brosaplanella]

Can you push your recent changes so I can see the full error messages from the automatic tests?

[Saransh-cpp]

Yes sure, I was trying to use values=, but that didn't work out as well.

[brosaplanella]

I tried it locally and it seems the issue is with [A.h]. It seems that this runs fine:

        # Cell capacity [A.h] deprecated
        with self.assertRaisesRegex(ValueError, "Cell capacity"):
            pybamm.ParameterValues({"Cell capacity [A.h]": 1,
                                    "Nominal cell capacity [A.h]": 1})
        with self.assertWarnsRegex(DeprecationWarning, "Cell capacity"):
            pybamm.ParameterValues({"Cell capacity [A.h]": 1})

[Saransh-cpp]

Thank you so much!

[brosaplanella]

@all-contributors add @Saransh-cpp for code and tests

[allcontributors]

@brosaplanella

I've put up a pull request to add @Saransh-cpp! 🎉

[brosaplanella]

Looks good, thanks @Saransh-cpp! We have a bit PR to merge, so I will wait to merge this one until the other is done as it will be easier to handle it this way.

[Saransh-cpp]

@brosaplanella okay, thanks!