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

Add functionalities to cache data when interpolating on nonmatching meshes #2414

Merged
merged 38 commits into from
Jan 23, 2023
Merged

Add functionalities to cache data when interpolating on nonmatching meshes #2414

merged 38 commits into from
Jan 23, 2023

Conversation

massimiliano-leoni
Copy link
Contributor

@massimiliano-leoni massimiliano-leoni commented Oct 25, 2022

After merging #2245 dolfinx can interpolate discrete functions defined on different meshes. This is, however, quite costly.
This PR adds a class that performs this interpolation caching as much data as possible so that interpolating again between the same two functions, when their meshes haven't moved, is much cheaper.

Some timings for the same example as in the discussion of #2245 [mesh with 750'000 cells]

serial
Interpolating       7.311494
Interpolating again 0.572593

n = 6 processes
Interpolating       1.772976
Interpolating again 0.164332

@jhale
Copy link
Member

jhale commented Oct 28, 2022

Would it be possible to implement this not using a class? For example, the interpolate method could return a map containing the cache which could then be passed as an optional argument. std::optional might help.

@jhale
Copy link
Member

jhale commented Oct 28, 2022

Another (perhaps better) option would be to bring the expensive call to determine_point_ownership outside the function.

@IgorBaratta
Copy link
Member

Another (perhaps better) option would be to bring the expensive call to determine_point_ownership outside the function.

There's also opportunity of improvement in eval, which is quite expensive (evaluating basis at interpolation points).
Maybe, for starters, we could use a more explicit way of caching data, with the functions that are already exist (in a demo for example).

@massimiliano-leoni
Copy link
Contributor Author

@jhale I guess it is possible to avoid using a class. Out of ignorance curiosity, what is the matter with using a class?

@jhale
Copy link
Member

jhale commented Dec 19, 2022

@massimiliano-leoni A core design principles in DOLFINx is to avoid implicit state (as much as possible) and to prefer functional approaches as much as possible. I think what you want to achieve could be more straightforwardly achieved by making the expensive call to determine_point_ownership explicit.

@massimiliano-leoni
Copy link
Contributor Author

@jhale I understand that it's a design decision, but I was wondering for my own knowledge what the reason behind the decision is!

@garth-wells
Copy link
Member

@jhale I understand that it's a design decision, but I was wondering for my own knowledge what the reason behind the decision is!

Avoiding, where possible, objects with (mutable) state leads to simpler code (not need to check state), avoids inconsistent state, avoids hidden memory 'leaks' (not technically leaks, but objects that might hold a lot of memory and which isn't obvious to the user), and thread safety.

This is why we prefer explicit approaches first. Sometimes it might then be appropriate to build a convenience class/function on top of the low-level implementation.

For this particular case, it seems simplest to me that the user calls geometry::determine_point_ownership and then uses the data. This give more flexibility, e.g. a case where the caller knows the cells and just needs to determine the ranks.

@massimiliano-leoni
Copy link
Contributor Author

@garth-wells Thanks for the explanation. I implemented it the way you and @jhale suggested. Is it better now?

cpp/dolfinx/fem/interpolate.h Outdated Show resolved Hide resolved
cpp/dolfinx/fem/interpolate.h Outdated Show resolved Hide resolved
cpp/dolfinx/fem/interpolate.h Show resolved Hide resolved
cpp/dolfinx/fem/interpolate.h Outdated Show resolved Hide resolved
cpp/dolfinx/fem/interpolate.h Outdated Show resolved Hide resolved
cpp/dolfinx/fem/interpolate.h Outdated Show resolved Hide resolved
cpp/dolfinx/fem/interpolate.h Show resolved Hide resolved
@jhale
Copy link
Member

jhale commented Jan 12, 2023

Pretty much I think, just a few style things I left in the review. Another thing we do is try to make expensive calls explicit, so I don't think we would include a code path where the data is not pre-computed in this low-level interface.

…imiliano-leoni/dolfinx into mleoni/cache-repeated-interpolation
@jhale
Copy link
Member

jhale commented Jan 13, 2023

I think this is close. How does it all look from the Python interface?

@massimiliano-leoni massimiliano-leoni changed the title Add a class to cache data when interpolating multiple times Add functionalities to cache data when interpolating multiple times Jan 13, 2023
@massimiliano-leoni massimiliano-leoni changed the title Add functionalities to cache data when interpolating multiple times Add functionalities to cache data when interpolating on nonmatching meshes Jan 13, 2023
@massimiliano-leoni
Copy link
Contributor Author

I think this is close. How does it all look from the Python interface?

I think I updated it accordingly. At least, the CI is satisfied. I suggest someone quickly goes through my additions because I'm not very familiar with pybind11 and writing Python wrappers so I might have overlooked something.

@jhale jhale merged commit 8d55f95 into FEniCS:main Jan 23, 2023
@massimiliano-leoni massimiliano-leoni deleted the mleoni/cache-repeated-interpolation branch January 23, 2023 16:27
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 this pull request may close these issues.

4 participants