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

from Architectures to Grids to Models #2078

Merged
merged 55 commits into from
Dec 8, 2021
Merged

Conversation

simone-silvestri
Copy link
Collaborator

@simone-silvestri simone-silvestri commented Nov 22, 2021

In this PR some structures are refractored such that

  • all grids depend on the architecture
  • the models takes only the grid as an input

To do so, the architecture is now a positional argument of the grid constructor (so it can be extended by MultiArch) as @glwagner suggested. The flow of the script will be something like this now:

arch = ...
grid = Grid(arch, FT; kwargs...)
model = NonhydrostaticModel(grid = grid, kwargs...)

Some comments on the MultiArch architecture...

  • MultiArch has now a child_architecture field specified on construction
  • MultiArch still depends on a topology, so, a way to go about it it is to first define a topology and then
    arch = MultiArch(CPU(), ranks=ranks, topology=topology); grid = (arch, topology=topology, kwargs...) or to define an architecture without topology and then pass it to the grid. Take care that in this second case it is not guaranteed that arch == grid.architecture as the architecture in the grid is constructed from topology(grid)
  • the grid constructed by grid(arch::MultiArch; kwargs...) will be a local grid. To reconstruct the global grid use reconstruct_global_grid(grid)

To do

  • as of now a field, given grid(arch::MultiArch; kwargs...) as an argument, is constructed locally, which is no problem if you set! a field from a function but will create problems if you want to set! from a global array
  • Explore new FFT options to allow more flexibility in the distributed NonhydrostaticModel
  • allow GPU halo_passing to support MultiArch(GPU(); kwargs...)

Closes #2073
Closes #1825

@navidcy navidcy added GPU 👾 Where Oceananigans gets its powers from grids 🗺️ user interface/experience 💻 labels Nov 22, 2021
Comment on lines 14 to 17
function PressureSolver(arch::MultiArch, grid::RegRectilinearGrid)
full_grid = reconstruct_global_grid(grid)
if arch.ranks[1] == 1 && full_grid.Nx == full_grid.Ny # we would have to allow different settings
return DistributedFFTBasedPoissonSolver(arch, full_grid, grid)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function PressureSolver(arch::MultiArch, grid::RegRectilinearGrid)
full_grid = reconstruct_global_grid(grid)
if arch.ranks[1] == 1 && full_grid.Nx == full_grid.Ny # we would have to allow different settings
return DistributedFFTBasedPoissonSolver(arch, full_grid, grid)
function PressureSolver(arch::MultiArch, local_grid::RegRectilinearGrid)
global_grid = reconstruct_global_grid(local_grid)
if arch.ranks[1] == 1 && global_grid.Nx == global_grid.Ny # we would have to allow different settings
return DistributedFFTBasedPoissonSolver(arch, global_grid, local_grid)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can take of that global_grid.Nx == global_grid.Ny actually because of #2079


struct MultiCPU{G, R, I, ρ, C, γ} <: AbstractCPUArchitecture
distributed_grid :: G
struct MultiArch{A, R, I, ρ, C, γ} <: AbstractMultiArchitecture
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep the tradition of verbosity and use MultiArchitecture here?

Copy link
Member

Choose a reason for hiding this comment

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

Also wondering if MultiArchitecture is even best. What about DistributedArchitecture? Seems a bit more to the point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, actually there is no need for an AbstractMultiArchitecture either

struct MultiCPU{G, R, I, ρ, C, γ} <: AbstractCPUArchitecture
distributed_grid :: G
struct MultiArch{A, R, I, ρ, C, γ} <: AbstractMultiArchitecture
child_arch :: A
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
child_arch :: A
child_architecture :: A

?

Probably best to remain consistent throughout regarding nomenclature...

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

Totally epic. Hard to review it all so I just skimmed it.

My main comments regard names. It's too easy to introduce multiple names for the same thing (like arch and architecture) so I think we should be disciplined to prevent this disease from infecting our code.

Rolling it over in my head a few times I think MultiArchitecture (or MultiArch) sounds a bit off (I realize this is not entirely a contribution of this PR, since the name was present before). But perhaps DistributedArchitecture is better? We can also save this change for a future PR.

I also think it will really help future developers if we firmly enforce the names local_grid, global_grid in our code to minimize confusion (distributed computing is confusing enough...)

@francispoulin
Copy link
Collaborator

Greg has a good point. If we are talking about using multiple CPUs or GPUs, then DistributedArchitecture would perhaps be more precise.

@simone-silvestri simone-silvestri merged commit 18ee21b into main Dec 8, 2021
@simone-silvestri simone-silvestri deleted the ss/grid_depening_on_arch branch December 8, 2021 01:12
glwagner added a commit that referenced this pull request Dec 14, 2021
Notably we don't have a version that captures #2078.
@glwagner glwagner mentioned this pull request Dec 14, 2021
glwagner added a commit that referenced this pull request Dec 14, 2021
Notably we don't have a version that captures #2078.
aramirezreyes added a commit to aramirezreyes/Oceananigans.jl that referenced this pull request Dec 27, 2021
This makes the docstring consistent with the changes to the constructor from CliMA#2078
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GPU 👾 Where Oceananigans gets its powers from grids 🗺️ user interface/experience 💻
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we store architecture in grid?
4 participants