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

Major refactor of FieldBoundaryConditions #1843

Merged
merged 61 commits into from
Jul 15, 2021
Merged

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Jul 9, 2021

This PR introduces a major refactor of FieldBoundaryConditions and changes the user API.

The grid and field location are no longer necessary to build FieldBoundaryConditions. This means that the constructors TracerBoundaryConditions, UVelocityBoundaryConditions, etc, are gone. Instead we have only FieldBoundaryConditions, which is used like

u_bcs = FieldBoundaryConditions(top=FluxBoundaryCondition(-1e-4), bottom=FluxBoundaryCondition((x, y, t, u) = -1e-3 * u, field_dependencies=:u))

T_bcs = FieldBoundaryConditions(top=FluxBoundaryCondition(1e-8))

There's also a new constructor AuxiliaryFieldBoundaryConditions which takes the place of ComputedFieldBoundaryConditions and is now the default for Field and ComputedField. The difference is that AuxiliaryFieldBoundaryConditions have nothing instead of an ImpenetrableBoundaryCondition in Bounded directions when they're located on faces.

Under the hood the FieldBoundaryConditions struct is "flattened" so there's no more CoordinateBoundaryConditions. In addition, a new field immersed is added to FieldBoundaryConditions in addition to east, west, etc. This will be used to support boundary conditions on immersed boundaries in the future.

There's still quite a few validation experiments to clean up and probably some lingering issues with tests, but I was able to get the examples to run so I thought it was the right time to open the PR.

On the API changes: it seemed like a positive change that we don't need grid in the boundary conditions constructor, and that we only have one name FieldBoundaryConditions rather than 4. However, if others feel they liked the old API, we can add convenience functions TracerBoundaryConditions, etc back to the source.

@navidcy
Copy link
Collaborator

navidcy commented Jul 9, 2021

Ok, this definitely needs bumping up a minor release

@navidcy
Copy link
Collaborator

navidcy commented Jul 9, 2021

Great job. I’ll try to help out cleaning up the rest.

@glwagner glwagner requested a review from navidcy July 9, 2021 21:19
@navidcy
Copy link
Collaborator

navidcy commented Jul 14, 2021

@glwagner, regarding the

ERROR: LoadError: UndefVarError: bc not defined

in the Docs, is it related to
JuliaDocs/Documenter.jl#228
perhaps?

@navidcy
Copy link
Collaborator

navidcy commented Jul 14, 2021

I believe it should be OK now. Commit e39c509 was the crucial fix here.

Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve.

I applaud the tests/jldoctests.

I'm a bit skeptical whether I have the expertise to be as critical.

@navidcy
Copy link
Collaborator

navidcy commented Jul 14, 2021

@glwagner
Copy link
Member Author

@glwagner, regarding the

ERROR: LoadError: UndefVarError: bc not defined

in the Docs, is it related to
JuliaDocs/Documenter.jl#228
perhaps?

Nice find!

@glwagner
Copy link
Member Author

I'm going to clean up the validation experiments for the new syntax and then this is ready.

@glwagner
Copy link
Member Author

glwagner commented Jul 14, 2021

I approve.

I applaud the tests/jldoctests.

I'm a bit skeptical whether I have the expertise to be as critical.

The main point of approval here is whether we think the API change is positive. I think you are one of the best people anywhere to judge this change.

The other change, which is adding a field immersed, is innocuous. We have to have some way of implementing boundary conditions on immersed boundaries and this is one solution to that. Probably its hard to say for anyone what the "best" solution is (but alternative designs are welcome).

Note that it's possible to implement a field immersed without the API change. But I felt the API change was positive / important (I feel we've got to squash technical debt ASAP in all cases or I think it will bite us worse in the future).

@navidcy
Copy link
Collaborator

navidcy commented Jul 15, 2021

We should wait for this to be merged before we move on with #1847, right?

@glwagner
Copy link
Member Author

When we merge this we will have to update the example in #1847 but otherwise this is just an aesthetics change and has no impact on output, so we don't need to wait for it.

@glwagner glwagner merged commit 6e39d3f into master Jul 15, 2021
@navidcy navidcy deleted the glw/refactor-field-bcs branch September 29, 2021 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants