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

cache symbol shapes for testing #749

Closed
rtimms opened this issue Nov 27, 2019 · 9 comments · Fixed by #780
Closed

cache symbol shapes for testing #749

rtimms opened this issue Nov 27, 2019 · 9 comments · Fixed by #780
Assignees

Comments

@rtimms
Copy link
Contributor

rtimms commented Nov 27, 2019

For some bigger problems discretising all of the variables can be slow. It would be good to have the option to only discretise the variables actually used by the model and then be able to discretise a user specified list of variables after solving for post-processing etc.

@rtimms
Copy link
Contributor Author

rtimms commented Nov 27, 2019

after further investigation it turns out the thing that is very slow to discretise is the flux in the particles, so might be worth looking there instead

@valentinsulzer
Copy link
Member

discretisation reuses parts of the tree it's already discretised so in theory it shouldn't scale too badly with number of variables, but some specific variables might cause problems. One potential bottleneck is hashing of large arrays in Array.set_id (requires care as not all array-hashing methods give unique hashes, but we might not be optimally re-using hashes)

@rtimms
Copy link
Contributor Author

rtimms commented Nov 27, 2019

yeah it is just those two variables in particular (particle flux in neg and pos electrode). I'll look properly at some point, but it could be the way we handle boundary conditions in gradient. at the moment we loop over secondary points and keep concatenating onto a bcs_vector, indexing the boundary condition as appropriate. I'm not sure if this is actually the slow part or not. In any case, in big 2+1D problems there are a lot of particles if using DFN...

@valentinsulzer
Copy link
Member

Ah interesting, yeah that could be very inefficient - maybe that part could be defined as another matrix multiplication?

@rtimms
Copy link
Contributor Author

rtimms commented Nov 27, 2019

yeah i think it definitely can be written that way. might take a look and do it as part of #741, will probably be next week though

@TomTranter
Copy link
Contributor

Last week i did some profiling and found that line 690 in discretisation is always calling test_shape. This slows things down considerably. Do you want to address that in this issue or should I create a separate one?

@rtimms
Copy link
Contributor Author

rtimms commented Dec 4, 2019

it turns out the thing that was being slow that prompted me to make this issue was the handling of boundary conditions when you have lots of particles. i’m planning to fix that as part of #741. i think this issue can either be closed or you could use this issue to fix the slow test shape. will the solution to that to be to only call test shape in debug mode or something similar?

@valentinsulzer
Copy link
Member

It might be easy to speed up test_shape by doing some caching, e.g.:

  • replace evaluate_for_shape by _evaluate_for_shape in all the expression tree classes
  • in Symbol, do something like:
def evaluate_for_shape(self):
        """Evaluate expression tree to find its shape. For symbols that cannot be
        evaluated directly (e.g. `Variable` or `Parameter`), a vector of the appropriate
        shape is returned instead, using the symbol's domain.
        See :meth:`pybamm.Symbol.evaluate()`
        """
        try:
            return self._saved_evaluate_for_shape
        except AttributeError:
            self._saved_evaluate_for_shape = self._evaluate_for_shape()
            return self._saved_evaluate_for_shape

Hopefully this will provide some speed up, as currently evaluate_for_shape re-evaluates every child in the tree each time a parent is created (i.e. scales exponentially with tree depth)

@rtimms rtimms changed the title Only discretise necessary variables cache symbol shapes for testing Dec 13, 2019
@rtimms
Copy link
Contributor Author

rtimms commented Dec 13, 2019

I've changed the way bcs_vector is constructed as part of #741, so this issue can be used to cache shapes to speed up evaluate_for_shape

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants