Minor clean-ups to the FastScape interfaces. #6723
Conversation
source/mesh_deformation/fastscape.cc
Outdated
| { | ||
| // add a declare for kd | ||
| std::vector<double> bedrock_transport_coefficient_array(fastscape_array_size); | ||
| // TODO: where is the receive for this??? |
There was a problem hiding this comment.
I think it is inside fill_fastscape_arrays a function that is only executed on rank 0 and receives the transmissions from all other ranks. Of course this is quite confusing to read, maybe it could be restructured.
There was a problem hiding this comment.
But maybe @Djneu could confirm this?
There was a problem hiding this comment.
Oh, I meant to take that out of the patch again. I did want to investigate the issue, but not as part of this PR.
There was a problem hiding this comment.
Yes that sounds correct, I supposed it used to be called directly within the overarching function but I moved it into fastscape_fill_arrays when I was doing an initial clean up of the code. Would it make more sense to also call fastscape_fill_arrays on the non-root processors, but then within that function have processors that aren't the root send the data?
There was a problem hiding this comment.
I'll take a look at some point in the future. Or if you'd like to either change it or at least document in this place where the corresponding receive is, that's fine with me. I don't think it is particularly important.
The place mainly caught my eye because it uses Ssend, which is quite expensive compared to the other send variants. Do you recall why you picked that one?
There was a problem hiding this comment.
It has been awhile, but if I recall during initial testing we were actually getting relatively significant speedups from using ssend over the normal send. Individually ssend from each processor was a bit slower, however using send there would often be occasional delays for an individual processor and with these the total time for send was longer than ssend.
This was in a much earlier version of the code, and we only tested ssend and send so I am not sure if it would still be relevant.
There was a problem hiding this comment.
@Djneu Huh, strange. ssend definitely has a higher overhead.
Do we have tests these days that execute these sends? If so, I could try to change this to Isend, which is generally the more efficient approach.
There was a problem hiding this comment.
Unfortunately I don't have any good tests for this, I would be really curious to see how much of a difference all the different sends make though. Maybe I can try to set some simple model at some point that we can use to test these.
source/mesh_deformation/fastscape.cc
Outdated
| throw aspect::QuietException(); | ||
|
|
||
| // This is called solely so we can set the timer and will return immediately. | ||
| // (The kd array is a dummy array.) |
There was a problem hiding this comment.
Ah, good catch kd is now named bedrock_transport_coefficient_array!
There was a problem hiding this comment.
Yes, I think it refers to
There was a problem hiding this comment.
How about renaming the array to contain the word "dummy"? Then you can delete the comment. Alternatively: "// this array is unused:"
There was a problem hiding this comment.
Looking at this again, I am not sure why I sent mesh_velocity_z for all the other arrays and then the kd array. As this only enters for the timer and then exits, maybe a dummy should be sent for all of them?
There was a problem hiding this comment.
I renamed the variable as suggested. I will track the other issues separately.
|
I made the changes as suggested. Everything not already done will be tracked in follow-up issues. |
|
👍 Looks good to me. |
4 participants
I was reading through this code with @danieldouglas92 last week and when you look, you will always find places that can be improved.
@Djneu FYI.