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

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

Closed
mhvwerts opened this issue Apr 8, 2024 · 8 comments · Fixed by #38
Closed

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

mhvwerts opened this issue Apr 8, 2024 · 8 comments · Fixed by #38

Comments

@mhvwerts
Copy link
Member

mhvwerts commented Apr 8, 2024

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 mhvwerts changed the title Store (a reference to) the boundary conditions as a property of a cell variable Store the boundary conditions as a property of a cell variable Apr 9, 2024
@mhvwerts
Copy link
Member Author

mhvwerts commented Apr 9, 2024

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
Copy link
Member Author

mhvwerts commented Apr 10, 2024

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
Copy link
Member

simulkade commented Apr 10, 2024

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
Copy link
Member

[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
Copy link
Member Author

[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
Copy link
Member

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
Copy link
Member Author

mhvwerts commented Apr 10, 2024

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants