FEniCS / ufl

UFL - Unified Form Language
https://fenicsproject.org
GNU Lesser General Public License v3.0
104 stars 64 forks source link

Replace multifunctions with class methods #273

Open mscroggs opened 6 months ago

mscroggs commented 6 months ago

I propose that we replace algorithms implemented using multifunctions with class methods. This will allow UFL to be more extensible, as a user can then implement alrogithms for their own class defined outside UFL without having to edit the UFL core.

For an example, see https://github.com/FEniCS/ufl/pull/271.

dham commented 6 months ago

How do we know the LRU cache is big enough?

mscroggs commented 6 months ago

How do we know the LRU cache is big enough?

Good question. We should probably use lru_cache(maxsize=None) (which is what cache is from Python 3.9 onwards, #272)

dham commented 6 months ago

Isn't that a memory leak?

mscroggs commented 6 months ago

I need to read up on lru_cache more. I was trying to avoid writing our own cache decorator but maybe that's unavoidable

dham commented 6 months ago

I think there is a tough lifetimes issue going on here. UFL needs to hold onto references for the duration of a tree traversal, lest the DAG explode. On the other hand, some UFL objects will be subclassed and carry large amounts of data (Coefficient and Matrix) so it's important that stray references to those objects are not retained, otherwise quite bad memory leaks can occur (this is a current pyadjoint issue, BTW). I think that probably means that the caches have to have a life of exactly one tree traversal. I think that would mean still having something like multifunction, but delegating the implementation of the individual cases back to the objects, so that you keep extensibility.

mscroggs commented 6 months ago

I've added a hastily implemented caching function to #271.

Caching function is in https://github.com/FEniCS/ufl/blob/mscroggs/apply_restrictions/ufl/core/caching.py

Can then be called like this:

https://github.com/FEniCS/ufl/blob/e9efc6c7b303e74dc093481ac6e2242e342dbbe1/ufl/algorithms/apply_restrictions.py#L37-L42

dham commented 6 months ago

That's not recursion safe. Imagine someone puts some UFL inside an ExternalOperator, and then takes a derivative. The derivative application method of the external operator might well itself call the derivative application visitor.

Another issue with this approach is that it assumes that we know in advance what the full set of DAG visitors is, because if someone wants to do something to a UFL expression that we didn't think of, they will have to go through and subclass every single UFL type to add the new method.

A further issue is that it's not at all clear how you specify traversal orders other than post-order.

I think that the right way to do this is till to have a dedicated tree visitor (i.e. a multifunction but without using the UFL type system). @wence- described how to do this well in #35. Note that this is completely compatible with having a lot of visitors built in to UFL expression as methods. That bit of this proposal isn't the issue.

mscroggs commented 6 months ago

274 is an updated version of my PR that uses class methods with a copy of the old traversal methods