Avoiding distributed fill_halo_regions! in dynamics by extending the halos#88
Conversation
|
I think this can be closed since the solution in #89 is much better |
|
I reopen this PR given the state of distributed sea ice in #96. This might be the solution. We can proceed once we have a profile. |
Refactor halo region filling to only process local halos.
|
@taimoorsohail I think this PR might be ready. I need to add some tests, but I think I will do a whole test overhaul in a new PR and add some distributed tests there. This might increase the efficiency of distributed sea ice tremendously. |
simone-silvestri
changed the title
fill_halo_regions!fill_halo_regions! in dynamics by extending the halos
| bottom_momentum_stress = nothing, | ||
| free_drift = nothing, | ||
| solver = SplitExplicitSolver(150), | ||
| solver = SplitExplicitSolver(grid; substeps=150), |
There was a problem hiding this comment.
Thought --- should we distinguish between the halo size and number of substeps? This would allow adaptive time-stepping; users just have to select the maximum number of substeps they expect to need. This could be combined with a max time step option in the TimeStepWizard
There was a problem hiding this comment.
In other words the abstraction here is baking in an equivalence between substeps and halo points, but it need not be
There was a problem hiding this comment.
Yeah this is kind of the downside of this method. We could think about giving a "maximum halo", which would limit the amount of substeps (and in turn the maximum allowable timestep for the TimeStepWizard).
|
Impressive how simple it is, I had to double check I wasn't missing anything. I suggest adding tests though before merging... |
|
Can we tag a release for this commit? I would but don't have the permissions for this repo. Thanks :) |
3 participants
fill_halo_regions!is massively expensive when kernels are small.This is because there is a lot of type instability in the
fill_halo_regions!that slows down the computation and the momentum kernels are very small so the performance hit is felt massively when running a small computation like a one-degree simulation.This PR is just a draft that tries to implement the same strategy as the free surface in oceananigans to avoid fill halo regions.
However, I feel like it is possible to revisit
fill_halo_regions!to make sure it is lightning fast under certain assumption, so does not really need to be merged, I will try to figure out how to speed up fill halo regions. We can keep this as last resort