FiniteVolumeTransportPhenomena / PyFVTool

Finite volume toolbox in Python
GNU Lesser General Public License v2.1
18 stars 4 forks source link

Store the boundary conditions as a property of a cell variable #36

Closed mhvwerts closed 6 months ago

mhvwerts commented 6 months ago

In the discussion accompanying PR #34, the suggestion was made to include (a reference to) the boundary conditions (subclass of BoundaryConditionsBase) as a property of the CellVariable to which these boundary conditions apply. At present, the definition of the boundary conditions is made separately from the cell variable, and applied explicitly in the user code, which requires recalling the specific boundary conditions object every time.

User code may become more simple, readable and perhaps also more robust, if the boundary conditions were included in the cell variable. This might simply be achieved by including something like self.BC = BC in CellVariable.__init__() [1], followed by adapting any other code where BCs are now specified explicitly such that it uses the stored BC where relevant.

This should also be the opportunity to review and revise CellVariable.bc_to_ghost() and BC2GhostCells() whose functioning is not very clear. AFAICS, these two methods are not used anywhere in the entire PyFVTool code base. Also, I find it strange that their name is a reference to boundary conditions (BCs), but no explicit boundary conditions are used by their code. These methods calculate the new value for the ghost cells (= boundary cells?) [2] by taking the average of that ghost cell with neighbouring inner cell and then storing that new value in the ghost cell. Perhaps, we can simply remove these methods, since they are not used?

[1] Actually: self.BC = arg[0] / self.BC = None, depending on how it is called. The CellVariable.__init__() code requires some further cleaning up I think...

[2] Should we use consistent naming: use either the term 'boundary cells', or the term 'ghost cells' exclusively? Perhaps I am missing a subtlety here? Boundary cell is perhaps ambiguous: it may refer to the ghost cell or to the outermost inner cell?

mhvwerts commented 6 months ago

Thinking about the inclusion of the information on boundary conditions inside the CellVariable object, I realized that it may be an idea to have the boundary conditions fully handled by the CellVariable object: creation of BoundaryConditions object stored inside the CellVariable structure, definition of the a, b, c etc., pre-calculation/updating of the BoundaryConditionsTerm matrix + RHS etc. etc.

This would lead to a more logical workflow, where first the cell variable is created, followed by definition of the boundary conditions if required (only solution variables need boundary conditions).

I think that such a solution may even be implemented without breaking the existing infrastructure.

mhvwerts commented 6 months ago

Concerning CellVariable.bc_to_ghost() and CellVariable.BC2GhostCells() (cell.py), these have perhaps been superseded by cellValuesWithBoundaries (boundary.py)? It seems to serve a similar function, and does indeed take into account the defined boundary conditions when assigning values to the ghost cells.

simulkade commented 6 months ago

Concerning CellVariable.bc_to_ghost() and CellVariable.BC2GhostCells() (cell.py), these have perhaps been superseded by cellValuesWithBoundaries (boundary.py)? It seems to serve a similar function, and does indeed take into account the defined boundary conditions when assigning values to the ghost cells.

Indeed, it is the case. In the development sprint, I replaced some of the old functions with more +at least in my head+ logical ones. This is one of those cases.

This would lead to a more logical workflow, where first the cell variable is created, followed by definition of the boundary conditions if required (only solution variables need boundary conditions).

I totally agree. I cannot remember any use cases that a boundary exist without a CellVariable (well, for steady-state problems we have the BC before having the CellVariable but later the solution will be a cell variable).

simulkade commented 6 months ago

[2] Should we use consistent naming: use either the term 'boundary cells', or the term 'ghost cells' exclusively? Perhaps I am missing a subtlety here? Boundary cell is perhaps ambiguous: it may refer to the ghost cell or to the outermost inner cell?

I am in favor of ghost cells.

mhvwerts commented 6 months ago

[2] Should we use consistent naming: use either the term 'boundary cells', or the term 'ghost cells' exclusively? Perhaps I am missing a subtlety here? Boundary cell is perhaps ambiguous: it may refer to the ghost cell or to the outermost inner cell?

I am in favor of ghost cells.

So am I.

simulkade commented 6 months ago

The CellVariable.init() code requires some further cleaning up I think

What I prefer to do is to initialize the CellVariable with a boundary condition (Neumann) and later adjust the values of the -infamous- a, b, c. It is the workflow that we have more or less in all examples. We can at some point write some utility functions to do the adjustment in more pythonic ways, e.g. by accepting dictionaries; for example:

BC = {
    "left": {"type": "Dirichlet", "value": 0.0},
    "right": {"type": "Neumann", "value": 0.0},
}
mhvwerts commented 6 months ago

I agree.

I have a relatively simple plan for embedding the boundary condition structure in the CellVariable, creating it if it is not supplied externally. It will be initialized with default "no flux" (Neumann) values. Then, it can be fine-tuned via the usual left right etc. a, b, c mechanism. Finally, we call a method which we may name apply_BCs that re-calculates the ghost cell values (via cellValuesWithBoundaries) and even pre-calculates the BoundaryConditionsTerms M and RHS.

This could later be embellished in more pythonic ways, as you suggest.

simulkade commented 6 months ago

I agree.

I have a relatively simple plan for embedding the boundary condition structure in the CellVariable, creating it if it is not supplied externally. It will be initialized with default "no flux" (Neumann) values. Then, it can be fine-tuned via the usual left right etc. a, b, c mechanism. Finally, we call a method which we may name apply_BCs that re-calculates the ghost cell values (via cellValuesWithBoundaries) and even pre-calculates the BoundaryConditionsTerms M and RHS.

This could later be embellished in more pythonic ways, as you suggest.

Love the idea! Simple and elegant.