Small improvements to PerturbationAdvectionOpenBoundaryCondition interface
#4394
Conversation
src/BoundaryConditions/perturbation_advection_open_boundary_matching_scheme.jl
Outdated
Show resolved
Hide resolved
src/BoundaryConditions/perturbation_advection_open_boundary_matching_scheme.jl
Outdated
Show resolved
Hide resolved
src/BoundaryConditions/perturbation_advection_open_boundary_matching_scheme.jl
Show resolved
Hide resolved
src/BoundaryConditions/perturbation_advection_open_boundary_matching_scheme.jl
Outdated
Show resolved
Hide resolved
|
@jagoosw can you please write a couple of sentences about |
|
Sorry it took me so long to get to this. I have added some sentences about the timescales but am not sure what to put for the stepping options. I think we should have removed the forward stepping before merging because I can't convince myself that it is valid. |
I'm okay with that. @glwagner are you okay with this? |
src/BoundaryConditions/perturbation_advection_open_boundary_matching_scheme.jl
Outdated
Show resolved
Hide resolved
|
|
||
| Creates a `PerturbationAdvectionOpenBoundaryCondition` with a given `outflow_timescale` and | ||
| `inflow_timescale`. `backward_step` determines whether we assume a backward and forward time | ||
| discretization. For details about this method, refer to the docstring for `PerturbationAdvection`. |
There was a problem hiding this comment.
the docstring doesn't describe what val is. I'm also confused, would a better name than val help?
There was a problem hiding this comment.
val is the exterior value of the normal valocity. The one we're relaxing the flow to (with inflow_timescale during inflow and with outflow_timescale with during outflow). I agree it isn't a very intuitive name. Maybe just exterior_value?
src/BoundaryConditions/perturbation_advection_open_boundary_matching_scheme.jl
Show resolved
Hide resolved
src/BoundaryConditions/perturbation_advection_open_boundary_matching_scheme.jl
Outdated
Show resolved
Hide resolved
|
I'm ok with any improvement. I don't know what "forward stepping" is but with more detail I can give feedback on that. |
src/BoundaryConditions/perturbation_advection_open_boundary_matching_scheme.jl
Outdated
Show resolved
Hide resolved
At the moment you can make the boundary do a forward or backwards Euler step and backwards is the default. I did some testing with forward stepping (I can't remember why) but I was never convinced that it was sound because we have the interior values at almost |
I also did a bit of testing and (also don't remember why, but) ended up not liking the forward step too much. So I support removing it :) |
| Δt = ifelse(isinf(Δt), 0, Δt) | ||
|
|
||
| ūⁿ⁺¹ = getbc(bc, l, m, grid, clock, model_fields) | ||
|
|
||
| uᵢⁿ = @inbounds getindex(u, boundary_indices...) |
There was a problem hiding this comment.
@jagoosw this isn't critical but I'd appreciate some clarity here. In your derivation you arrive at the equation
uⁿ⁺¹ = (uᵢⁿ + Ũuᵢ₋₁ⁿ⁺¹ + Uⁿ⁺¹τ̃) / (1 + τ̃ + U)
and from it, I understand that uᵢⁿ is the normal velocity at the previous time-step. However, here you're defining uᵢⁿ using the current time-step, no? Am I misunderstanding something?
There was a problem hiding this comment.
This is where we get the previous value of the boundary point since it hasn't been stepped yet so its still at the previous time step, and then put uⁿ⁺¹ in the same place to bring the whole field to the current time step.
There was a problem hiding this comment.
Ah I see, thanks for the clarification. Just to clarify even further, the way time-stepping works is that the interior of the flow is time-stepped first, then we do the boundaries?
…gans.jl into tc/perturb-adv-obc-tweaks
No description provided.