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

Replace multifunctions with class methods #273

Open
mscroggs opened this issue Apr 22, 2024 · 8 comments
Open

Replace multifunctions with class methods #273

mscroggs opened this issue Apr 22, 2024 · 8 comments
Labels

Comments

@mscroggs
Copy link
Member

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 #271.

@mscroggs mscroggs added this to newFL Apr 22, 2024
@mscroggs mscroggs moved this to Aims & design principles in newFL Apr 22, 2024
@dham
Copy link
Collaborator

dham commented Apr 22, 2024

How do we know the LRU cache is big enough?

@mscroggs
Copy link
Member Author

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
Copy link
Collaborator

dham commented Apr 22, 2024

Isn't that a memory leak?

@mscroggs
Copy link
Member Author

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
Copy link
Collaborator

dham commented Apr 22, 2024

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
Copy link
Member Author

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:

def apply_restrictions(expression: ApplyRestrictions):
"""Propagate restriction nodes to wrap differential terminals directly."""
caching.initialise_cache("apply_restrictions")
result = expression.apply_restrictions()
caching.clear_cache("apply_restrictions")
return result

@dham
Copy link
Collaborator

dham commented Apr 23, 2024

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Aims & design principles
Development

No branches or pull requests

2 participants