Parrallel Unstructured Mesh Deformation Interface class - #7083
Conversation
|
You'll have to run the indentation script: |
bangerth
left a comment
There was a problem hiding this comment.
I wonder what the proper protocol is -- @tjhei and I designed and implemented these classes. We probably shouldn't review this patch :-)
We could say that you (@danieldouglas92) should just review your own pull request ;-) Alternatively, perhaps @MFraters or @Minerallo or @Djneu would be willing to take a look?
|
@bangerth I'll quickly go through and at least clean up the documentation and then ping the others once this is ready for a proper review |
4c25fd4 to
245c029
Compare
MFraters
left a comment
There was a problem hiding this comment.
Looks generally good. The current openLEM integration works through a moving grid. I see no reason why not, but can you confirm that it is possible (and efficient) to update the evaluation points in between getting the velocities from aspect and interpolating the velocites back to aspect? Can you add a test which does that?
|
can we call it |
Yes, I think that that is probably a more descriptive name. |
it involves MPI communication but I expect this to be scalable (we use this in a different deal.II code on 100k MPI ranks).
We probably should do that. We can triggers this with a simple example where we have mesh adaptivity for example. |
45a39e4 to
0228853
Compare
|
@MFraters I added a test which shows that the update() function resets the evaluation_points()! |
|
|
||
| if (my_rank == 0) | ||
| { | ||
| // Combine data coming from all ranks and determine closest evaluation point for each DoF: |
There was a problem hiding this comment.
From this part, this already combines all ranks so later on we don't need to gather everything down to rank 0 in the individual landscape evolution model interfaces?
There was a problem hiding this comment.
Add more documentation about this
There was a problem hiding this comment.
This piece of code is only an implementation detail (and might be replaced in the future) to deterministically make a decision "which points is closest". It forgets about any information exchanged at the end of this function.
@danieldouglas92 can you add a comment like this?
| * surface, rather than on the currently deformed top surface | ||
| * that describes the current elevation map. | ||
| * | ||
| * @note This function sets up communication structures that |
There was a problem hiding this comment.
Will I need to think about resetting the evaluation points on mesh refinement for something like fastscape when its mesh will remain fixed?
There was a problem hiding this comment.
We need to rebuild the mapping of external evaluation points and degrees of freedom even if the fastscape mesh stays fixed. This already happens using a signal.
Djneu
left a comment
There was a problem hiding this comment.
Hi all, I mostly just had some questions so I understand everything better, but I think it looks like it does everything that would be needed! If I understand, if fastscape.cc is eventually updated to use this the workflow would be something like:
-
Set up what the fastscape mesh will be and return the values with set_evaluation_points.
-
compute_velocity_constrains_on_boundary will then evaluate the solution values everywhere (and have them combined on rank 0), and in compute updated velocities I can use these values to find the velocity differences as well as set up all the compositional kd/kf values needed.
0228853 to
d84490f
Compare
0638271 to
7ceea4a
Compare
|
So in the FastScape C++ coupling we used Utilities::MPI::isend() and irecv() with Wolfgang last year. The current interface from what I see use .compress(), this hide how the information is re-distributed. I wonder if we dont need to access this information for future applications ... ? |
7ceea4a to
d7199d9
Compare
|
@Minerallo A new comment has been added about the .compress function |
d6b8452 to
e13205c
Compare
tjhei
left a comment
There was a problem hiding this comment.
I only have a few minor comments.
|
|
||
| if (my_rank == 0) | ||
| { | ||
| // Combine data coming from all ranks and determine closest evaluation point for each DoF: |
There was a problem hiding this comment.
This piece of code is only an implementation detail (and might be replaced in the future) to deterministically make a decision "which points is closest". It forgets about any information exchanged at the end of this function.
@danieldouglas92 can you add a comment like this?
1b00a6b to
33c1573
Compare
MFraters
left a comment
There was a problem hiding this comment.
Can you clarify what happens to the output vector when points are outside of the domain (as discussed)? Maybe initialize the vector with NaN.
Otherwise looks good to me.
|
|
||
| if (!remote_point_evaluator->all_points_found()) | ||
| { | ||
| this->get_pcout() << "WARNING: not all evaluation points were found inside the domain!" << std::endl; |
There was a problem hiding this comment.
Can you make a this a debug only output?
Co-authored-by: Timo Heister <heister@clemson.edu> Co-authored-by: Wolfgang Bangerth <bangerth@colostate.edu>
33c1573 to
abc0d64
Compare
|
@MFraters I addressed your comments! |
MFraters
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. Looks good to me now.
Yes, this is exactly how we think it would look like. |
@tjhei @bangerth if there is a proper way to give you credit for this please let me know. This is adding the external_tool_interface mesh deformation plugin that you guys wrote for the ASPECT-Landlab project with the tests that do not require python/landlab.