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

[Bug]: "electrode diffusivity" -> "particle diffusivity" is still breaking change on develop when using pre-24.5 pybamm.ParameterValues #4176

Closed
ejfdickinson opened this issue Jun 13, 2024 · 0 comments · Fixed by #4179 or #4267
Assignees
Labels
bug Something isn't working release blocker Issues that need to be addressed before the creation of a release

Comments

@ejfdickinson
Copy link

ejfdickinson commented Jun 13, 2024

PyBaMM Version

develop

Python Version

3.9.13

Describe the bug

Since #4072 re-inverted the logic fix in #3871, the release candidate on develop causes the rename of "electrode diffusivity" -> "particle diffusivity" to be a breaking change. Parameter sets using the old key names do not simulate in the 24.5 RC.

Steps to Reproduce

import pybamm

parameter_values = pybamm.ParameterValues("Chen2020")

# Dummy a pybamm.ParameterValues object with a legacy parameter name
parameter_values.update(
    {
        "Negative electrode diffusivity [m2.s-1]": parameter_values["Negative particle diffusivity [m2.s-1]"],
    },
    check_already_exists=False,
)
del parameter_values["Negative particle diffusivity [m2.s-1]"]

# Try to run a simulation with the old-style parameter set
model = pybamm.lithium_ion.DFN()
experiment = pybamm.Experiment(
    [
        "Discharge at 1C until 3 V",
    ]
)
sim = pybamm.Simulation(
    model,
    parameter_values=parameter_values,
    experiment=experiment,
)

# KeyError provoked here on call into pybamm.ParameterValues with new key name
sol = sim.solve()

Relevant log output

No response

@ejfdickinson ejfdickinson added the bug Something isn't working label Jun 13, 2024
@agriyakhetarpal agriyakhetarpal added the release blocker Issues that need to be addressed before the creation of a release label Jun 13, 2024
@BradyPlanden BradyPlanden self-assigned this Jun 14, 2024
js1tr3 pushed a commit to js1tr3/PyBaMM that referenced this issue Aug 12, 2024
* fix: revert logic change for failing 'particle diffusivity' key, updt. check_parameter_values to rename depreciated key

* fix: removes bpx remapping requirement, creates 'particle diffusivity' without destroying 'electrode diffusivity', updts tests to verify functionality

* Add changelog

* Apply suggestions from code review

Co-authored-by: Ferran Brosa Planella <[email protected]>
Co-authored-by: Agriya Khetarpal <[email protected]>

---------

Co-authored-by: Ferran Brosa Planella <[email protected]>
Co-authored-by: Agriya Khetarpal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release blocker Issues that need to be addressed before the creation of a release
Projects
None yet
3 participants