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

Restores open boundary condition functionality #3937

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

jagoosw
Copy link
Collaborator

@jagoosw jagoosw commented Nov 18, 2024

The refactoring in #3834 broke flat extrapolation open boundary conditions. This restores them.

Closes #3975

@glwagner
Copy link
Member

shoudl we add a test to make sure it keeps working?

@simone-silvestri
Copy link
Collaborator

a test is a good idea.

@@ -61,7 +61,7 @@ end

const c = Center()

@inline function _fill_west_open_halo!(j, k, grid, ϕ, bc::FEOBC, loc, clock, model_fields)
@inline function _fill_west_halo!(j, k, grid, ϕ, bc::FEOBC, loc, clock, model_fields)
Δx₁ = xspacing(1, j, k, grid, c, c, c)
Copy link
Collaborator

@simone-silvestri simone-silvestri Nov 18, 2024

Choose a reason for hiding this comment

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

Suggested change
Δx₁ = xspacing(1, j, k, grid, c, c, c)
Δx₁ = Δxᶜᶜᶜ(1, j, k, grid)

Probably better to use the pointwise operators from here (and in the rest of this file)

@navidcy navidcy changed the title Fixs flat extrapolaiton open boundary condition Fixes flat extrapolaiton open boundary condition Nov 19, 2024
@jagoosw jagoosw changed the title Fixes flat extrapolaiton open boundary condition Restores open boundary condition functionality Dec 6, 2024
@jagoosw
Copy link
Collaborator Author

jagoosw commented Dec 6, 2024

I might be missing something but I think that the changes in #3834 make it so that open halos are filled even when fill_boundary_normal_velocities is set to false. I can't see what mechanism would stop that happening @simone-silvestri ?

Edit: I have now added it

# Returns the boundary conditions a specific side for `FieldBoundaryConditions` inputs and
# a tuple of boundary conditions for `NTuple{N, <:FieldBoundaryConditions}` inputs
for dir in (:west, :east, :south, :north, :bottom, :top)
extract_side_bc = Symbol(:extract_, dir, :_bc)
@eval begin
@inline $extract_side_bc(bc) = bc.$dir
@inline $extract_side_bc(bc) = retrieve_bc(bc.$dir)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@simone-silvestri I think we need this analogously to retrieve_open_bc

@jagoosw jagoosw added bug 🐞 Even a perfect program still has bugs testing 🧪 Tests get priority in case of emergency evacuation boundary conditions 🏓 labels Dec 6, 2024
@jagoosw
Copy link
Collaborator Author

jagoosw commented Dec 10, 2024

@glwagner @simone-silvestri I think this should be all done now and we should merge ASAP since open boundaries are currently broken

Copy link
Collaborator

@simone-silvestri simone-silvestri left a comment

Choose a reason for hiding this comment

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

sounds good!

@jagoosw jagoosw marked this pull request as ready for review December 10, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
boundary conditions 🏓 bug 🐞 Even a perfect program still has bugs testing 🧪 Tests get priority in case of emergency evacuation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected FlatExtrapolationBoundaryCondition behaviour in validation script cylinder.jl
3 participants