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]: ys too large when computing sensitivities #3130

Closed
Mrzhang-hub opened this issue Jul 10, 2023 · 6 comments · Fixed by #3313
Closed

[Bug]: ys too large when computing sensitivities #3130

Mrzhang-hub opened this issue Jul 10, 2023 · 6 comments · Fixed by #3313
Assignees
Labels
bug Something isn't working difficulty: medium Will take a few days priority: medium To be resolved if time allows

Comments

@Mrzhang-hub
Copy link

Mrzhang-hub commented Jul 10, 2023

PyBaMM Version

23.5

Python Version

3.9

Describe the bug

  File "D:\ProgramData\Anaconda3\lib\site-packages\pybamm\solvers\solution.py", line 108, in __init__
    self.check_ys_are_not_too_large()
  File "D:\ProgramData\Anaconda3\lib\site-packages\pybamm\solvers\solution.py", line 311, in check_ys_are_not_too_large
    y_var = y[model.variables[var.name].y_slices[0]]
AttributeError: 'Multiplication' object has no attribute 'y_slices'

Steps to Reproduce

import numpy as np
import os
import pybamm
import pandas as pd
import matplotlib.pyplot as plt
model = pybamm.lithium_ion.SPMe(
    options={
        "thermal": "lumped",
        "dimensionality": 0,
        "cell geometry": "arbitrary",
        "electrolyte conductivity": "integrated",
    },
    name="TSPMe",
)
param = pybamm.ParameterValues("Chen2020")
param.update({'Current function [A]':'[input]'})
param.update({"Negative electrode diffusivity [m2.s-1]": '[input]'})
param.update({"Positive electrode diffusivity [m2.s-1]": '[input]'})
geometry = model.default_geometry
param.process_geometry(geometry)
param.process_model(model)
var = pybamm.standard_spatial_vars
var_pts = {var.x_n:30,var.x_s:30,var.x_p:30,var.r_n:10,var.r_p:10}
mesh = pybamm.Mesh(geometry,model.default_submesh_types,var_pts)
disc = pybamm.Discretisation(mesh,model.default_spatial_methods)
disc.process_model(model)
t_eval = np.linspace(0,3600,100)
solver = pybamm.CasadiSolver(mode='safe',atol=1e-6,rtol=1e-3)
start_params = [0.15652, 3.54e-14, 1.01e-13]
solution = solver.solve(model,t_eval,inputs={'Current function [A]': start_params[0],
                "Negative electrode diffusivity [m2.s-1]": start_params[1],
                "Positive electrode diffusivity [m2.s-1]": start_params[2]},
                calculate_sensitivities=True)
fig,ax = plt.subplots(1,3,figsize=(10,5))
ax[0].plot(t_eval,solution['Current [A]'].entries)
ax[0].set_xlabel(r'$t$')
ax[0].set_ylabel(r'$Current [A]$')
ax[1].plot(t_eval,solution['Terminal voltage [V]'].entries)
ax[1].set_xlabel(r'$t$')
ax[1].set_ylabel(r'$Terminal voltage [V]$')
ax[2].plot(t_eval,solution['Terminal voltage [V]'].sensitivities['Current function [A]'])
ax[2].set_xlabel(r'$t$')
ax[2].set_ylabel(r'$sensitivities of Terminal voltage wrt Current function$')
plt.tight_layout()
plt.show()

Relevant log output

No response

@brosaplanella
Copy link
Member

I have now updated the MWE, it seems to be an issue with the calculation of the sensitivities. @martinjrobins is this a bug?

@brosaplanella
Copy link
Member

I have now noticed that this has to do with the variables being too large. Can you set calculate_sensitivities=False and see if the issue persists (so we know if it is with solving the model or with the sensitivities only).

@brosaplanella brosaplanella added the needs-reply Needs further information from the author and may be closed if no response is received label Jul 27, 2023
@Mrzhang-hub
Copy link
Author

  1. Yes, I have also noticed that y > pybamm.settings.max_y_value when check_ys_are_not_too_large, but I do not know why it is that large and why this error AttributeError: 'Multiplication' object has no attribute 'y_slices' appears.
  2. When I set calculate_sensitivities=False, it runs normally. Thus it is caused by sensitivities only. The following figure is the simulation result.
    image

@brosaplanella
Copy link
Member

Thanks for the update. I will change the title to something more informative.

@brosaplanella brosaplanella changed the title [Bug]: 'Multiplication' object has no attribute 'y_slices' [Bug]: ys too large when computing sensitivities Jul 28, 2023
@brosaplanella brosaplanella added difficulty: medium Will take a few days priority: medium To be resolved if time allows labels Jul 28, 2023
@valentinsulzer valentinsulzer removed the needs-reply Needs further information from the author and may be closed if no response is received label Aug 8, 2023
@brosaplanella brosaplanella added the in-progress Assigned in the core dev monthly meeting label Sep 4, 2023
martinjrobins added a commit that referenced this issue Sep 7, 2023
@martinjrobins martinjrobins mentioned this issue Sep 7, 2023
6 tasks
@martinjrobins
Copy link
Contributor

not sure why this code was trying to get the y_slice from the variable, but I've changed so that it is getting it from model.y_slices, which fixes the issue, see pr #3313

@martinjrobins
Copy link
Contributor

not sure why this code was trying to get the y_slice from the variable, but I've changed so that it is getting it from model.y_slices, which fixes the issue, see pr #3313

Ahhh, this was for models that don't need to be discretised. I've also done a fix for this

martinjrobins added a commit that referenced this issue Sep 7, 2023
martinjrobins added a commit that referenced this issue Sep 7, 2023
@martinjrobins martinjrobins removed the in-progress Assigned in the core dev monthly meeting label Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working difficulty: medium Will take a few days priority: medium To be resolved if time allows
Projects
Development

Successfully merging a pull request may close this issue.

4 participants