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]: post-processing variables require pybamm.Symbol to have a mesh #4637

Closed
martinjrobins opened this issue Dec 2, 2024 · 6 comments · Fixed by #4644
Closed

[Bug]: post-processing variables require pybamm.Symbol to have a mesh #4637

martinjrobins opened this issue Dec 2, 2024 · 6 comments · Fixed by #4644
Labels
bug Something isn't working

Comments

@martinjrobins
Copy link
Contributor

martinjrobins commented Dec 2, 2024

PyBaMM Version

develop

Python Version

3.10

Describe the bug

Adding a variable to a pre-built pybamm model causes a failure in pybamm.solvers.processed_variable.py::process_variable() because it doesn't have a mesh attribute

Steps to Reproduce

  1. create a pybamm model (e.g. SPM)
  2. manually add a new variable (model.variables.update({"name": expression}))
  3. solve the model then try and access the variable (sol["name"])

Relevant log output

  File "/home/mrobins/git/PyBOP/pybop/problems/fitting_problem.py", line 133, in evaluate
    return {
  File "/home/mrobins/git/PyBOP/pybop/problems/fitting_problem.py", line 134, in <dictcomp>
    signal: sol[signal].data
  File "/home/mrobins/git/PyBOP/env/lib/python3.10/site-packages/pybamm/solvers/solution.py", line 710, in __getitem__
    self.update(key)
  File "/home/mrobins/git/PyBOP/env/lib/python3.10/site-packages/pybamm/solvers/solution.py", line 590, in update
    self._update_variable(variable)
  File "/home/mrobins/git/PyBOP/env/lib/python3.10/site-packages/pybamm/solvers/solution.py", line 640, in _update_variable
    var = pybamm.process_variable(
  File "/home/mrobins/git/PyBOP/env/lib/python3.10/site-packages/pybamm/solvers/processed_variable.py", line 881, in process_variable
    mesh = base_variables[0].mesh
AttributeError: 'Power' object has no attribute 'mesh'
@martinjrobins martinjrobins added the bug Something isn't working label Dec 2, 2024
@martinjrobins
Copy link
Contributor Author

this doesn't normally pop up as we seem to add mesh somewhere in the pipeline, but it seems to occur if you add the new variable after the model is discretised

@rtimms
Copy link
Contributor

rtimms commented Dec 2, 2024

We could try to process the variable with an empty disc, e.g.

disc = pybamm.Discretisation()
disc.process_symboll(variable)

We do this in the solver if you try to solve a model that isn't yet discredited, and throw a DiscretisationError if it fails.

Seems like it would be best practice to add the variable to the model before discretising though.

@martinjrobins
Copy link
Contributor Author

I think the main issue with our current approach is that mesh is not an attribute of the Symbol class, its just something that gets added somewhere along the line, which is not good. In general all class attribute should be created in the __init__, not afterwards. I would suggest just adding self.mesh = None in Symbol.__init__

@rtimms
Copy link
Contributor

rtimms commented Dec 2, 2024

Got it, makes sense. There are a lot of places where attributes get added outside of the __init__

@martinjrobins
Copy link
Contributor Author

Got it, makes sense. There are a lot of places where attributes get added outside of the __init__

I know :( but this was a particularly blatent example that I thought we should clean up

@rtimms
Copy link
Contributor

rtimms commented Dec 3, 2024

Agreed!

martinjrobins added a commit that referenced this issue Dec 3, 2024
rtimms pushed a commit that referenced this issue Dec 3, 2024
* bug: make sure symbol always has required mesh attribute

* #4637 add to changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants