(0.98.0) Modify interface for OpenBoundaryConditions with schemes and use Tuples in boundary_mass_fluxes

[tomchor]

This apparently came up in a conversation between @glwagner and @navidcy and was mentioned in #4674, so I'm opening this. This PR:

• Modifies the interface for creating OpenBoundaryConditions with "matching schemes", which now are just called schemes
• Removes the "experimental" warning for PerturbationAdvectionOpenBoundaryCondition now that it has more rigorous testing and seems to be working properly
• Removes FlatExtrapolation based on the discussion in Trying to run a regional simulation with river inflow #4703 (comment)
• Uses tuples (instead of arrays) to hold the open boundaries in model.boundary_fluxes.

[glwagner]

I also took the liberty of exporting PerturbationAdvectionOpenBoundaryCondition at the top level. Let me know if that's not wanted.

Can we change the name to just OpenBoundaryCondition? I think we should use OpenBoundaryCondition and implement a generic constructor that can be used for the different cases flexibly. PerturbationAdvectionOpenBoundaryCondition is too long and also isn't really self-describing, so we don't benefit from the verbosity.
I would make this API change instead of exporting a new name.

[glwagner]

What are the benefits of a NamedTuple vs type here

[tomchor]

Not sure I understand the question. What do you mean by type?

[glwagner]

a type other than NamedTuple --- a custom struct, which is an alternative to using a NamedTuple

[tomchor]

I guess just the simplicity of using a common type that everyone's familiar with. I don't see disadvantages of using NamedTuples.

[tomchor]

I also took the liberty of exporting PerturbationAdvectionOpenBoundaryCondition at the top level. Let me know if that's not wanted.

Can we change the name to just OpenBoundaryCondition? I think we should use OpenBoundaryCondition and implement a generic constructor that can be used for the different cases flexibly. PerturbationAdvectionOpenBoundaryCondition is too long and also isn't really self-describing, so we don't benefit from the verbosity. I would make this API change instead of exporting a new name.

Sure, something like OpenBoundaryCondition(U; matching_scheme=PerturbationAdvection())?

[glwagner]

I also took the liberty of exporting PerturbationAdvectionOpenBoundaryCondition at the top level. Let me know if that's not wanted.

Can we change the name to just OpenBoundaryCondition? I think we should use OpenBoundaryCondition and implement a generic constructor that can be used for the different cases flexibly. PerturbationAdvectionOpenBoundaryCondition is too long and also isn't really self-describing, so we don't benefit from the verbosity. I would make this API change instead of exporting a new name.

Sure, something like OpenBoundaryCondition(U; matching_scheme=PerturbationAdvection())?

right, with OpenBoundaryCondition() defaulting to impenetrable perhaps?

Also what is a "perturbation advection scheme"? One thing to consider for the name is what the different alternatives might be so that we can choose a name that meaningfully distinguishes between them. From what I understand, we either have a scheme that strictly prescribes the boundary velocity, or a scheme that can evolve it, is that right?

[tomchor]

I also took the liberty of exporting PerturbationAdvectionOpenBoundaryCondition at the top level. Let me know if that's not wanted.

Can we change the name to just OpenBoundaryCondition? I think we should use OpenBoundaryCondition and implement a generic constructor that can be used for the different cases flexibly. PerturbationAdvectionOpenBoundaryCondition is too long and also isn't really self-describing, so we don't benefit from the verbosity. I would make this API change instead of exporting a new name.

Sure, something like OpenBoundaryCondition(U; matching_scheme=PerturbationAdvection())?

right, with OpenBoundaryCondition() defaulting to impenetrable perhaps?

👍

Also what is a "perturbation advection scheme"? One thing to consider for the name is what the different alternatives might be so that we can choose a name that meaningfully distinguishes between them. From what I understand, we either have a scheme that strictly prescribes the boundary velocity, or a scheme that can evolve it, is that right?

Yes, and the schemes that predict what the boundary velocity can be from the flow (rather then prescribe/impose it) typically do so in order to minimize reflection. There is a lot of different nomenclature here, so I don't know what the best name is. When originally implementing this @jagoosw chose to call these schemes "matching schemes", but I haven't found that name elsewhere in the literature (@jagoosw feel free to chime in here; perhaps radiation_scheme is more intuitive to understand?). I think they just call these different types of BCs radiation or nonreflective boundary conditions. For the record, here's what Durran 2010 says about it:

(So a note is that, us calling boundary conditions that prescribe the normal velocity without trying to minimize reflection as an OpenBoundaryConditions goes against common nomenclature.)

And back to your point, PerturbationAdvection is what we ended up calling the "matching scheme" that @jagoosw implemented. I remember we had some back and forth about that name back then, but I couldn't find that scheme in the literature so I'm not sure what it is usually called.

[jagoosw]

As I remember, we settled on "matching scheme" since the scheme abstractly matches the interior physics with some prescribed boundary physics. I think that radiation schemes are a subset of this because an open boundary might e.g. be prescribing an inflow, and I gather that radiation schemes gained their name from their derivation from approximating the wave propagation from the Helmholtz equation, which isn't universally how you might want to treat an open boundary. So I think calling them all radiation schemes is misleading. For example, the Stevens boundary condition (https://mitgcm.readthedocs.io/en/latest/phys_pkgs/obcs.html) has nothing to do with that derivation.

Similarly, for the "perturbation advection" we called it this because that describes what it does (i.e. it advects perturbations out of the domain with some mean velocity), this is similar to an Orlanski/radiation condition where the "mean" velocity is guessed by some particular procedure, or could be the boundary mean velocity like Simone describes here. If we can think of something else that describes what this does, then happy for it to be changed, but I think this is quite a good descriptor of what it can be used for. There is also a derivation in the docstring which kind of shows the advecting a pertubation idea.

[jagoosw]

Sure, something like OpenBoundaryCondition(U; matching_scheme=PerturbationAdvection())?

Given this API it could maybe just be called scheme?

[glwagner]

Sure, something like OpenBoundaryCondition(U; matching_scheme=PerturbationAdvection())?

Given this API it could maybe just be called scheme?

I like that. If there is only one kind of "scheme", it's better to avoid the adjective. Also just to rehash some very old conversations, originally we put off writing a unified API for OpenBoundaryCondition because we saw the implementation as experimental. So the unified API should be implemented as part of removing the "experimental" designation.

[navidcy]

If there is only one kind of "scheme", it's better to avoid the adjective.

I agree; and explain that kwarg scheme is about matching in the docstring.

[tomchor]

Sure, something like OpenBoundaryCondition(U; matching_scheme=PerturbationAdvection())?

Given this API it could maybe just be called scheme?

I like that. If there is only one kind of "scheme", it's better to avoid the adjective. Also just to rehash some very old conversations, originally we put off writing a unified API for OpenBoundaryCondition because we saw the implementation as experimental. So the unified API should be implemented as part of removing the "experimental" designation.

Not reviving anything old at all! It's one of the points I'm making in the top comment :)

If I understand correctly, I'll then remove the warning for this scheme.

[tomchor]

I think this is ready. I bumped the minor version since we're changing the user interface for for open BCs with schemes.

[tomchor]

@glwagner I implemented your suggestions. This should be good to go. The reactant tests are failing for some reason, but I think that's not related to anything here in this PR.

[ali-ramadhan]

Looks like the PR addresses review comments and it looks good to me! Just left a minor comment.

Should the perturbation_advection_open_boundary_scheme.jl file be renamed to just perturbation_advection.jl or perturbation_advection_scheme.jl now?

[tomchor]

The tests pass, with the exception of reactant tests and an error in the docs. None of these has anything to do with this PR. So @glwagner @navidcy @simone-silvestri can someone please merge this?

[glwagner]

I'll merge when the docs build successfully, which I think we should fix before taking 0.98.0. I fixed a doctest error so hopefully it'll build this time.

[navidcy]

Thanks @glwagner!

[tomchor]

I'll merge when the docs build successfully, which I think we should fix before taking 0.98.0. I fixed a doctest error so hopefully it'll build this time.

Sounds good, thanks. I thought we wanted to fix this error in another PR since it didn't come from the changes here which is why I didn't fix it myself. I'll keep an eye on the docs and fix any other errors in case that might pop up.