Add missing non-const `BoundaryRegionIterBase::prev` overload by ZedThree · Pull Request #3484 · boutproject/BOUT-dev

ZedThree · 2026-08-28T16:48:18Z

Missing non-const ref overload meant it was not possible to assign to point.prev(f)

@dschwoerer Please can this go into the Hermes-3 BOUT++ update PR?

Missing non-const ref overload meant it was not possible to assign to `point.prev(f)`

ZedThree · 2026-08-28T17:01:18Z

Actually there's at least one other function missing and some functions duplicated between the base and derived classes.

dschwoerer · 2026-08-31T06:55:26Z

Missing non-const ref overload meant it was not possible to assign to point.prev(f)

It might be beneficial to expose the <check> template parameter for this one.

@dschwoerer Please can this go into the Hermes-3 BOUT++ update PR?

I think it would be best if they first go into next, together with #3482

`is_lower()` and `ind()` now defined solely in base class

The previous interface made it impossible to get the correct boundaries at the point of calling `Coordinates::getYBoundary`. This refactors that and `YBoundary` creation so that it can simply be specified with two bools `lower_y/upper_y`. Getting theses from `Options` is now taken care of by `Coordinates::getYBoundary`.

ZedThree · 2026-09-03T17:21:52Z

I'm still not convinced check is needed at all, it's probably better to have separate valid_next and valid_prev variables to check in the separate directions, or to only check the point is valid when looking into the boundary.

There's also another big pain point in the design: getYBoundary() only takes YBndryType which is insufficient to control which boundaries are actually required at the point of calling. Additionally, YBndryType has variants sheath/not_sheath/all which also doesn't give sufficient control.

Instead I've refactored it to have two overloads: one that takes an Options and one that takes two bools. This allows us to create them as before, but then retrieving them doesn't require a lookup in Options.

We still only need to cache three YBoundary values for lower/upper/both, rather than sheath/not sheath/all, but no enum is exposed -- only the two bools are required.

dschwoerer · 2026-09-04T12:26:03Z

I'm still not convinced check is needed at all, it's probably better to have separate valid_next and valid_prev variables to check in the separate directions, or to only check the point is valid when looking into the boundary.

I think the implementation of the sheath BC will tell. I am fine either way.

There's also another big pain point in the design: getYBoundary() only takes YBndryType which is insufficient to control which boundaries are actually required at the point of calling. Additionally, YBndryType has variants sheath/not_sheath/all which also doesn't give sufficient control.

I do not understand. Why is it not sufficient? What control do you need? E.g. in #3483 I want to special handle sheath BCs. How do I do that with the new interface?

Instead I've refactored it to have two overloads: one that takes an Options and one that takes two bools. This allows us to create them as before, but then retrieving them doesn't require a lookup in Options.

But that is super confusing. Sometimes the API means yup/ydown. Sometimes core/SOL.
Does that not lead to lots of if/else in calling code? And it needs to parse the right bit of the options tree to figure out how the simulation is configured? I tried that for Hermes-3 initially, and it was leading to duplication in the config, or each component needed to parse over and over again the global options tree.

We still only need to cache three YBoundary values for lower/upper/both, rather than sheath/not sheath/all, but no enum is exposed -- only the two bools are required.

But figuring out which values to pass is tricky (outside of unit tests).

ZedThree · 2026-09-14T10:54:22Z

Why is it not sufficient? What control do you need?

Just so everyone is on the same page, the current behaviour works like this:

Coordinates is created, taking an Options from Mesh -- this will be the [mesh] section, and it's not possible to control this
- FA meshes will read lower/upper_y from this section
- FCI meshes will read inner/outer_x instead
Coordinates::getYBoundary only takes YBndryType, which can be either: sheath, not_sheath, or all
- all always uses both boundaries
- sheath takes the input values as given
- not_sheath inverts the input values -- setting lower_y = true in the input file and calling coords->getYBoundary(YBndryType::not_sheath) uses lower_y = false instead

So unless the YBndryType is an input value, all calling sites will be tied to the one input section, and it won't be possible to independently control whether or not an individual boundary is enabled at a particular side. Even if a component makes the boundary type an input value, the value to use will be dependent on the values in [mesh].

Concretely, with this input file:

[mesh]
lower_y = true
upper_y = true

it is not possible to turn off a single boundary condition at just one of the boundaries:

`YBndryType` value	Resulting `lower_y`	Resulting `upper_y`
`sheath`	`true`	`true`
`not_sheath`	`false`	`false`
`all`	`true`	`true`

Even in the cases where this works, sheath/not_sheath are not super descriptive about whether or not a boundary is enabled in a given direction. They're also not relevant for all physics models used in BOUT++ either.

The behaviour in this PR works instead as follows:

The object using the YBoundary is created, taking an appropriate Options
- this could be [mesh], or it could be a different section
- FA meshes will read lower/upper_y from this section
- FCI meshes will read inner/outer_x instead, as before
The object stores either:
- the input values of lower/upper_y during creation, and calls Coords::getYBoundary() with the stored values at runtime
- just stores the YBoundary and uses it directly

Now, it will be possible to set up boundary conditions that can be toggled independently, while still falling back to a global input value.

For example, one could have:

[mesh]
lower_y = true
upper_y = true

[A]
lower_y = false
# use upper_y from [mesh]

[B]
# use !mesh:lower_y and !mesh:upper_y
# behaves like `not_sheath` previously

So this PR gives us the most control while still a set of boundary conditions to be toggled together.

But that is super confusing. Sometimes the API means yup/ydown. Sometimes core/SOL.

I mean, I agree, but this retains the existing behaviour. How do you think it should behave?

each component needed to parse over and over again the global options tree.

I'm not quite sure what you mean here. Parsing the options tree should be done during setup, and not during runtime (that is, not during PhysicsModel::rhs() or Component::transform()), so any duplicated parsing should be very cheap, plus the parsing is cached, so there should be any performance cost here.

I think the correct behaviour for any physics model is, in general, that a given component or numerical object (Laplacian solver, time solver, etc) should read from a specific individual section, with a fallback to a more generic section.

So if a set of components should be linked together by default somehow, I would expect to see something like:

Options& opt = options == nullptr ? Options::root()["mesh"] : *options;

or

lower_y = options["lower_y"].with_default(
   Options::root()["mesh"]["lower_y"].with_default(false)
);

The first one being a bit more idiomatic BOUT++.

But figuring out which values to pass is tricky (outside of unit tests).

Ah, I think you're saying that we read in either lower/upper_y or inner/outer_x but don't actually return or store which values actually got read anywhere. We could store them in the YBoundary and add getters. Or we can also just store the YBoundary itself

dschwoerer

Ah, thanks, I had indeed missed that the API with options was your preferred way of doing things now 👍
I do agree that that is a cleaner implementation, but only if the interfaces with the two bools are not used by user code. That should be an implementation detail.

It would help if you would update the documentation as well, I think that still refers to the YBndryType?

We should make sure that the Options API is used, and not the one passing in bools, as they are confusing to use.

dschwoerer · 2026-09-14T11:11:57Z

+const bout::boundary::YBoundary& Coordinates::getYBoundary(bool lower_y,
+                                                           bool upper_y) const {


I think this should be private.
The API naming is confusing, as it seems to talk about lower_y and upper_y, but that also might be inner_x / outer_x

How about just lower/upper? Eventually, I'm hoping that something like this will be generic across all three dimensions, and so it will be good to have generic terminology

I do not think that would work. lower_y / upper_y will always be in the SOL, at least in the common topologies I have seen. For FCI it is just different.

I'm not sure what you mean. X and Y can have boundaries at either end, and Y may have more boundaries but these are still classified as either "lower" or "upper". FCI has parallel boundaries described as some combination of xin/xout and fwd/bwd; YBoundary operates over xin/xout.

Are you saying there are boundaries which could not be generically described as either lower or upper?

dschwoerer · 2026-09-14T11:12:11Z

 class YBoundary {
 public:
-  YBoundary(YBndryType type, Options* options_ptr, const Mesh& mesh);
+  YBoundary(const Mesh& mesh, bool lower_y, bool upper_y);


I think this should be private.
The API naming is confusing, as it seems to talk about lower_y and upper_y, but that also might be inner_x / outer_x

dschwoerer · 2026-09-14T11:12:25Z


 namespace bout::boundary {
-YBoundary::YBoundary(YBndryType type, Options* options_ptr, const Mesh& mesh)
+YBoundary::YBoundary(const Mesh& mesh, bool lower_y, bool upper_y)


I think this should be private.
The API naming is confusing, as it seems to talk about lower_y and upper_y, but that also might be inner_x / outer_x

dschwoerer · 2026-09-14T11:13:11Z

  dynamic_cast<FakeMesh*>(mesh)->createBoundaries();
  Field3D test = 1.0;
-  YBoundary sheath(YBndryType::all, nullptr, *mesh);
+  YBoundary sheath(*mesh, true, true);


I think this API should not be used (outside of unit tests, maybe)

Add missing non-const BoundaryRegionIterBase::prev overload

f31d95e

Missing non-const ref overload meant it was not possible to assign to `point.prev(f)`

ZedThree requested a review from dschwoerer August 28, 2026 16:48

ZedThree marked this pull request as draft August 28, 2026 17:00

ZedThree added 2 commits September 3, 2026 16:10

Fix inconsistency in methods of BoundaryIterators

697ec2d

`is_lower()` and `ind()` now defined solely in base class

ZedThree marked this pull request as ready for review September 3, 2026 17:21

dschwoerer reviewed Sep 14, 2026

View reviewed changes

ZedThree added 2 commits September 14, 2026 14:36

docs: Fix and expand YBoundary docs

afc28b1

Use consistent lower/upper naming for YBoundary

46dc2d0

Navigation Menu

Uh oh!

Add missing non-const `BoundaryRegionIterBase::prev` overload - #3484

Add missing non-const `BoundaryRegionIterBase::prev` overload#3484
ZedThree wants to merge 5 commits into
nextfrom
fix-missing-boundary-iterator-overload

ZedThree commented Aug 28, 2026

Uh oh!

ZedThree commented Aug 28, 2026

Uh oh!

dschwoerer commented Aug 31, 2026

Uh oh!

ZedThree commented Sep 3, 2026

Uh oh!

dschwoerer commented Sep 4, 2026

Uh oh!

ZedThree commented Sep 14, 2026

Uh oh!

dschwoerer left a comment

Uh oh!

dschwoerer Sep 14, 2026

Uh oh!

ZedThree Sep 14, 2026

Uh oh!

dschwoerer Sep 14, 2026

Uh oh!

ZedThree Sep 14, 2026

Uh oh!

dschwoerer Sep 14, 2026

Uh oh!

dschwoerer Sep 14, 2026

Uh oh!

dschwoerer Sep 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		const bout::boundary::YBoundary& Coordinates::getYBoundary(bool lower_y,
		bool upper_y) const {

Uh oh!

Conversation

ZedThree commented Aug 28, 2026

Uh oh!

ZedThree commented Aug 28, 2026

Uh oh!

dschwoerer commented Aug 31, 2026

Uh oh!

ZedThree commented Sep 3, 2026

Uh oh!

dschwoerer commented Sep 4, 2026

Uh oh!

ZedThree commented Sep 14, 2026

Uh oh!

dschwoerer left a comment

Choose a reason for hiding this comment

Uh oh!

dschwoerer Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

ZedThree Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

dschwoerer Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

ZedThree Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

dschwoerer Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

dschwoerer Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

dschwoerer Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants