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

PoissonBCs for pressure solver are redundant #311

Closed
glwagner opened this issue Jul 11, 2019 · 3 comments
Closed

PoissonBCs for pressure solver are redundant #311

glwagner opened this issue Jul 11, 2019 · 3 comments
Labels
cleanup 🧹 Paying off technical debt

Comments

@glwagner
Copy link
Member

The PoissonBCs:

https://github.com/climate-machine/Oceananigans.jl/blob/c961d3904700e73b1a4aebb71ebcc3f518693014/src/poisson_solvers.jl#L4

are redundant with the boundary conditions on the velocity fields. In other words, the pressure boundary conditions depend on the boundary conditions applied to the velocity field. There should not, therefore, be separate boundary condition types for the pressure solver. In particular, periodic boundary conditions on the velocity fields imply periodic boundary conditions on the pressure solver, while non-periodic boundary conditions on the velocity fields imply Neumann boundary conditions for the pressure solver.

The redundant implementation of boundary condition complicates model instantiation (the boundary conditions on the Poisson solver should be inferred from the boundary conditions on the velocity field, rather than specified independently), and is a source of fragility (because a physically invalid combination of boundary conditions on the velocity field and pressure solver can be specified).

@glwagner glwagner changed the title PoissonBCs are redundant PoissonBCs for pressure solver are redundant Jul 11, 2019
@ali-ramadhan
Copy link
Member

Agreed. We could address this when we refactor halo regions to dispatch on boundary conditions. Would then be good to add some code that asserts that the velocity boundary conditions are compatible with the boundary conditions we support (and with each other).

@ali-ramadhan ali-ramadhan added the cleanup 🧹 Paying off technical debt label Jul 12, 2019
@glwagner
Copy link
Member Author

Right. I think consistency checking would be a nice feature. I think that’d be best implemented in the constructor for ModelBoundaryConditions.

When consistent/valid boundary conditions are ensured, we can then safely infer the Poisson pressure solver type from the velocity boundary conditions without the need for an intermediate PoissonBCs type.

@ali-ramadhan
Copy link
Member

This was addressed in PR #589.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup 🧹 Paying off technical debt
Projects
None yet
Development

No branches or pull requests

2 participants