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

Factory-generated boundary conditions with new case file interface [Preview] #1264

Open
wants to merge 63 commits into
base: develop
Choose a base branch
from

Conversation

timofeymukha
Copy link
Collaborator

@timofeymukha timofeymukha commented Apr 30, 2024

Dear all, I think I've accumulated enough changes to launch this preview of the new bc handling. It is now almost fully implemented for the scalar (excluding the user field Dirichlet), but not yet for the fluid, therefore the don't merge. I do want the CI to run through, to see what complaints, I guess.

Major changes

The bc_t type

  • Adds an abstract interface for the constructor and the corresponding deferred routine init. Takes json_file and coef_t. Some derived types now use this to parse the JSON they need, e.g. the Dong condition, the Blasius, etc.
  • Some descendants also implement an init_from_componets in order to construct without JSON.
  • Add an abstract interface for finalization and the corresponding deferred routine finalize. Most types just call the finalize_base in bc_t, but symmetry and non_normal run their logic for determining the normal direction here.
  • Added generic apply for scalar and vector, which looks at whether we are on the device and calls the corresponding routine. Before, one had to use a bc_list to do that. I haven't actually used this yet, so maybe these will be removed later on.
  • In the step routines for the solvers, we need to be able to distinguish between weak and strong bcs. So bc_t no has an additional component strong = .true. to mark a bc as strong or weak.

The bc_list_t type

  • Moved to a separate file.
  • All the subroutines like bc_list_add are now TBPs, which simplifies use statements quite a bit, and is generally the way to go.
  • I mimic as much as possible the implementation of the field_list_t. Some differences remain though, e.g. for the bc_list we track both the capacity of the list and the number of allocated items. We need to think a bit more about this.
  • The apply_ routines are augmented with an optional strong parameter, which will lead to only applying strong or weak bcs. As a result, we can use a single list to store all the bcs and then call apply with either strong=.true. or strong=.false. when needed in the step routines.

The zero_dirichlet_t type

This is a rebranding of what used to the noslip_wall. The bc just assigns both vectors and scalars to zero. What I realized is that we have a lot of places in the code where we use a zero-valued Dirichlet for the purpose of marking boundaries for the linear solver routines. Using zero_dirichlet_t helps with clarity there.

The bc_factory subroutine

  • A factory that inits, marks, and finalizes a bc.
  • The marking is for a single zone, with the index taken from the JSON. (more on the new structure JSON below).
  • Currently set up to work with the scalar. What I realized is that construction of user-drives bcs requires knowledge of the scheme. So perhaps, what will happen is that each scheme has its own factory.
  • Does not support the field_dirichlet_t yet.

The scalar_scheme_t type

  • Uses a single bc_list called bcs to store the boundary conditions and constructs them with a factory. This results in quite a bit of cleanup here and there.
  • No longer uses bclabels.
  • Instead, the keyword boundary_conditions is used, which is an array of JSON objects, similar to how we do sources and simcomps. Each object has the type and zone_index keyword + other keywords needed, for that type.
  • The RB examples are updated to demonstrate the new layout.
            "boundary_conditions": [
            {
                "type": "dirichlet",
                "zone_index": 5,
                "value": 1.0
            },
            {
                "type": "dirichlet",
                "zone_index": 6,
                "value": 0.0
            },
            {
                "type": "user_pointwise"
            }
            ],

Remaining work

  • I don't like that with the current structure we have a lot of "no-op" routines in bc types, that don't implement either the scalar or vector apply. Or sometimes neither, as in the case of e.g. symmetry. One could split all types int scalar and vector, but mixed types would still fall out of this classification. Some mixed types, however, a just wrappers around several simple types as components. I have to look at this more.
  • The big work is to accommodate the new bc structure in fluid_scheme. I don't think it will become as "clean" as scalar because there are quite a bit of auxiliary bcs that are defined and will probably remain as additional components of the type.
  • Support for field_dirichlet should be added. I will wait for the small cleanup of those I've done in PR Slight update to field_dirichlet bcs #1261 to be merged first.

@timfelle
Copy link
Collaborator

timfelle commented Sep 3, 2024

Should we align the new BC's with the submodule structure we introduced recently before we merge this in?

@njansson
Copy link
Collaborator

njansson commented Sep 3, 2024

Should we align the new BC's with the submodule structure we introduced recently before we merge this in?

I don't think we need submodules for bc, unless we have some redundant factories that are in a separate file.

@timfelle
Copy link
Collaborator

timfelle commented Sep 3, 2024

I don't think we need submodules for bc, unless we have some redundant factories that are in a separate file.

Well this PR introduces a bc factory.

@njansson
Copy link
Collaborator

njansson commented Sep 3, 2024

I don't think we need submodules for bc, unless we have some redundant factories that are in a separate file.

Well this PR introduces a bc factory.

submodules it is. I just feel that for proper derived types, like dirichlet_t, it's good to have it in a own module

@timfelle
Copy link
Collaborator

timfelle commented Sep 3, 2024

submodules it is. I just feel that for proper derived types, like dirichlet_t, it's good to have it in a own module

I am not sure i follow. Do you suggest that direchlet_t should not be part of the general BC list, but instead handled manually on its own?

@njansson
Copy link
Collaborator

njansson commented Sep 3, 2024

submodules it is. I just feel that for proper derived types, like dirichlet_t, it's good to have it in a own module

I am not sure i follow. Do you suggest that direchlet_t should not be part of the general BC list, but instead handled manually on its own?

I mean like for ksp_t, base definition in one module, and factory in a submodule. But for the different flavours like cg and gmres, full modules are fine

@timfelle
Copy link
Collaborator

timfelle commented Sep 3, 2024

I mean like for ksp_t, base definition in one module, and factory in a submodule. But for the different flavours like cg and gmres, full modules are fine

I see, yes that makes sense. It would just be to have a factory for the abstract BC type in a submodule and then all the specializations are regular modules.

Sounds good to me.

@timofeymukha
Copy link
Collaborator Author

Hi, yes, I will have it in a submodule eventually!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
don't merge Don't merge yet! enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants