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

Parallel-safe mesh-to-mesh interpolation #128

Merged
merged 12 commits into from
Mar 21, 2024
Merged

Conversation

ddundo
Copy link
Member

@ddundo ddundo commented Mar 5, 2024

Closes #39.

Introduces parallel-compatible mesh-to-mesh interpolation (already wrapped up in firedrake as f_dest = Function(V_dest).interpolate(f_src) where two VertexOnlyMeshes are used as intermediaries).

@ddundo ddundo added bug Something isn't working enhancement New feature or request testing Extensions and improvements to the testing infrastructure labels Mar 5, 2024
@ddundo ddundo self-assigned this Mar 5, 2024
@ddundo
Copy link
Member Author

ddundo commented Mar 5, 2024

Hi @jwallwork23 and @stephankramer, after reading the whole day about VertexOnlyMeshes and petsc's star forest, I realised that the entire procedure has already been wrapped up super nicely within the usual interpolate and is in the main firedrake branch. So I am creating a super early draft pull request here to just demonstrate how simple it appears to be and to ask for a general strategy before doing more.

The biggest question is what to do with project - do we want to keep both options? I guess this PR should also address #123 and #44. We'd also need to make plotting parallel-compatible (could be addressed in #117). Anything else?

@ddundo ddundo linked an issue Mar 6, 2024 that may be closed by this pull request
@jwallwork23
Copy link
Member

Thanks for this @ddundo. Hopefully you learnt something while looking into VertexOnlyMesh and PETSc star forest!

I noticed the test failures on this PR so I tried out the mesh-to-mesh interpolation functionality in Firedrake myself. It seems like there are still some teething problems. I tried interpolating between two UnitSquareMeshes and got DofNotDefinedErrors. Weirdly, these still persisted even when I tweaked the target mesh so that all of its DoFs lie within the source domain. I then tried a case where the target mesh is already fully embedded within the source mesh, but still got DofNotDefinedErrors for all DoFs in the target space.

Regarding your question, let's not try to fix too many things in one PR. I think we should have both interpolation and projection options available.

@@ -324,7 +325,9 @@ def get_checkpoints(
checkpoints.append(
AttrDict(
{
field: project(sols[field], fs[i + 1])
field: firedrake.Function(fs[i + 1]).interpolate(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments:

  1. I think we should still support project, with an option to switch. I guess interpolate should be the default, if we can get it to work better in parallel.
  2. Do we expect the interpolate method to work for mixed spaces? (I haven't tested it.) The project method doesn't, which is why we had to write an interpolation module for Goalie.

@jwallwork23 jwallwork23 added this to the Version 1 milestone Mar 6, 2024
@ddundo
Copy link
Member Author

ddundo commented Mar 6, 2024

Thanks @jwallwork23! Regarding test failures and your review comment, actually when I run the tests locally only the gray_scott demo fails because interpolate raises the error NotImplementedError: UFL expressions for mixed functions are not yet supported..

  1. So yes, potentially I'll overload interpolate - I still have to test more
  2. About the other errors, I was surprised to see them here since they pass locally. Could you please try running the tests locally as well?

@jwallwork23
Copy link
Member

Hm no I get the same errors running locally:

====================================== short test summary info ======================================
FAILED test_demos.py::test_demos[burgers] - subprocess.CalledProcessError: Command '['/home/firedrake/firedrake/bin/python3', '/home/joe/sof...
FAILED test_demos.py::test_demos[burgers_time_integrated] - subprocess.CalledProcessError: Command '['/home/firedrake/firedrake/bin/python3', '/home/joe/sof...
FAILED test_demos.py::test_demos[gray_scott] - subprocess.CalledProcessError: Command '['/home/firedrake/firedrake/bin/python3', '/home/joe/sof...
FAILED test_demos.py::test_demos[burgers-hessian] - subprocess.CalledProcessError: Command '['/home/firedrake/firedrake/bin/python3', '/home/joe/sof...
FAILED test_demos.py::test_demos[burgers2] - subprocess.CalledProcessError: Command '['/home/firedrake/firedrake/bin/python3', '/home/joe/sof...
FAILED test_demos.py::test_demos[burgers_ee] - subprocess.CalledProcessError: Command '['/home/firedrake/firedrake/bin/python3', '/home/joe/sof...
FAILED test_demos.py::test_demos[burgers_oo] - subprocess.CalledProcessError: Command '['/home/firedrake/firedrake/bin/python3', '/home/joe/sof...
================ 7 failed, 121 passed, 3 skipped, 123 warnings in 286.62s (0:04:46) =================

@ddundo
Copy link
Member Author

ddundo commented Mar 6, 2024

Hmm thanks @jwallwork23, that's odd. I guess you are using the same docker image as the workflow here? Here is the results of my firedrake-status and the output of goalie test suite out.txt

---------------------------------------------------------------------------
|Package             |Branch                        |Revision  |Modified  |
---------------------------------------------------------------------------
|FInAT               |master                        |e3a869d   |False     |
|PyOP2               |master                        |e0a4d3a9  |False     |
|animate             |main                          |b39ff5c   |False     |
|concave_hull        |master                        |bdca55e   |False     |
|fiat                |master                        |dbc1c5d   |False     |
|firedrake           |master                        |63104ed4f |False     |
|goalie              |39_vom_parallel_interpolation |6faac5d   |False     |
|h5py                |firedrake                     |6b512e5e  |False     |
|icepack             |master                        |28eed36   |True      |
|libsupermesh        |master                        |84becef   |False     |
|loopy               |main                          |8158afdb  |False     |
|petsc               |jwallwork23/parmmg-rebased    |30d0933c23|False     |
|pyadjoint           |master                        |2c6614d   |False     |
|pytest-mpi          |main                          |8241bdc   |False     |
|tsfc                |master                        |90c20c5   |False     |
|ufl                 |master                        |054b0617  |False     |
---------------------------------------------------------------------------

I noticed the test failures on this PR so I tried out the mesh-to-mesh interpolation functionality in Firedrake myself. It seems like there are still some teething problems. I tried interpolating between two UnitSquareMeshes and got DofNotDefinedErrors. Weirdly, these still persisted even when I tweaked the target mesh so that all of its DoFs lie within the source domain. I then tried a case where the target mesh is already fully embedded within the source mesh, but still got DofNotDefinedErrors for all DoFs in the target space.

Could you please send me this script?

@jwallwork23
Copy link
Member

Yeah, I'm using the docker image locally rather than installing.

Attached scripts here.
interp.tar.gz

@ddundo
Copy link
Member Author

ddundo commented Mar 6, 2024

Thanks @jwallwork23, these both work for me locally. Could you please check if the docker image needs to be updated? I'm clueless for docker sadly :) but no rush! Sorry for the spam today

@jwallwork23
Copy link
Member

Thanks @jwallwork23, these both work for me locally. Could you please check if the docker image needs to be updated? I'm clueless for docker sadly :) but no rush! Sorry for the spam today

Yeah annoyingly the weekly automated Docker build hasn't worked recently for some reason (see mesh-adaptation/animate#79). I'll rebuild it now, push to DockerHub and then rerun the tests here.

@jwallwork23 jwallwork23 mentioned this pull request Mar 7, 2024
@ddundo
Copy link
Member Author

ddundo commented Mar 7, 2024

Yeah annoyingly the weekly automated Docker build hasn't worked recently for some reason (see pyroteus/animate#79). I'll rebuild it now, push to DockerHub and then rerun the tests here.

Thanks for doing that! Unfortunately there are still some failures here but everything passes on my local install.

@jwallwork23
Copy link
Member

Thanks for doing that! Unfortunately there are still some failures here but everything passes on my local install.

No worries. I notice that Gray-Scott is now fixed and all the errors are with Burgers. I wonder if the problem is with the demo...

@ddundo
Copy link
Member Author

ddundo commented Mar 7, 2024

No worries. I notice that Gray-Scott is now fixed and all the errors are with Burgers. I wonder if the problem is with the demo...

Well let me get docker running locally and investigate - I don't want to waste your time (and I need to finally give docker a go anyway :) ). I will ping you with updates/questions. Thanks for the help!

@ddundo ddundo force-pushed the 39_vom_parallel_interpolation branch from 39c7313 to a00cad2 Compare March 16, 2024 02:45
@ddundo ddundo marked this pull request as ready for review March 16, 2024 03:10
@ddundo
Copy link
Member Author

ddundo commented Mar 16, 2024

No rush at all with this, but it's ready for review when you have time please :) I just wanted to get it out of the way.

A few comments:

  1. I assigned interpolate/project to MeshSeq.transfer but there is already an existing function called transfer in indicate_errors. I don't think it's confusing but feedback is welcome
  2. The methodology in interpolation._transfer_adjoint is unfamiliar to me so please check that it and the docstring make sense

@ddundo ddundo removed the bug Something isn't working label Mar 16, 2024
Copy link
Member

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work @ddundo, thanks for this. Just a few requests.

goalie/mesh_seq.py Outdated Show resolved Hide resolved
goalie/interpolation.py Outdated Show resolved Hide resolved
goalie/interpolation.py Show resolved Hide resolved
goalie/interpolation.py Outdated Show resolved Hide resolved
@ddundo ddundo requested a review from jwallwork23 March 21, 2024 19:14
@ddundo
Copy link
Member Author

ddundo commented Mar 21, 2024

Thanks again @jwallwork23, ready for a re-review :)

Copy link
Member

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go, thanks @ddundo!

@ddundo ddundo merged commit ac3f431 into main Mar 21, 2024
1 check passed
@ddundo ddundo deleted the 39_vom_parallel_interpolation branch March 21, 2024 19:16
@ddundo
Copy link
Member Author

ddundo commented Mar 21, 2024

@acse-ej321 Just pinging you here to make you aware that interpolation is now used as the main transfer method :) so you might need to pass transfer_method="project" to MeshSeq in your code if you want to go back to projection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request testing Extensions and improvements to the testing infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parallel consistent interpolation
2 participants