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

More convenient BoundaryFunction functionality #699

Merged
merged 13 commits into from
Mar 16, 2020

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Mar 15, 2020

This PR makes it a bit easier to prescribe BoundaryFunctions by providing field-specific constructors. The field-specific constructors "know" about the locations of tracers and velocity fields and relieve the user of the (often confusing) task of specifying the BoundaryFunction location.
I also added a further wrapper around BoundaryCondition to permit patterns like

northern_velocity(x, z, t) = cos((x - sin(t)))
u_bcs = UVelocityBoundaryConditions(grid, north=UVelocityBoundaryCondition(Value, :y, northern_velocity))

It doesn't look perfect to me, but I do think it's progress over what the user had to do previously.

A potential source of confusion (?) is that UVelocityBoundaryCondition is only needed for simple boundary functions. We could alleviate this with more verbosity; eg UVelocityFunctionBoundaryCondition or something silly like that. But I'm hesitant because we already seem to have a verbosity problem. We need better design, not more words...

I've also added a parameters field to BoundaryFunction.

(It only just occurs to me that perhaps what we want are types like Tracer(), U(), V(), and W(). We might use these to classify boundary conditions and forcing functions... ?)

@codecov
Copy link

codecov bot commented Mar 15, 2020

Codecov Report

Merging #699 into master will increase coverage by 7.42%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
+ Coverage   71.12%   78.55%   +7.42%     
==========================================
  Files         122      122              
  Lines        2386     2485      +99     
==========================================
+ Hits         1697     1952     +255     
+ Misses        689      533     -156     
Impacted Files Coverage Δ
src/BoundaryConditions/BoundaryConditions.jl 100.00% <ø> (ø)
src/Fields/field_tuples.jl 91.17% <ø> (+5.46%) ⬆️
src/Fields/field.jl 75.90% <72.72%> (+10.42%) ⬆️
src/BoundaryConditions/boundary_function.jl 85.71% <94.11%> (+25.71%) ⬆️
...rc/BoundaryConditions/field_boundary_conditions.jl 92.00% <100.00%> (+8.00%) ⬆️
src/Models/incompressible_model.jl 88.88% <0.00%> (-11.12%) ⬇️
src/Logger.jl 79.16% <0.00%> (-9.73%) ⬇️
src/Solvers/solve_for_pressure.jl 93.33% <0.00%> (-6.67%) ⬇️
src/Utils/launch_config.jl 94.11% <0.00%> (-5.89%) ⬇️
...ntations/rozema_anisotropic_minimum_dissipation.jl 32.00% <0.00%> (-2.10%) ⬇️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bff7abc...b83618c. Read the comment docs.

Copy link
Member

@ali-ramadhan ali-ramadhan left a comment

Choose a reason for hiding this comment

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

Definitely an improvement if users don't have to worry about specifying the field location, although I guess it comes at the expense of extra constructors.

A potential source of confusion (?) is that UVelocityBoundaryCondition is only needed for simple boundary functions. We could alleviate this with more verbosity; eg UVelocityFunctionBoundaryCondition or something silly like that. But I'm hesitant because we already seem to have a verbosity problem. We need better design, not more words...

It only just occurs to me that perhaps what we want are types like Tracer(), U(), V(), and W(). We might use these to classify boundary conditions and forcing functions... ?

Yeah I'm not sure right now what a good user interface would look like. Those types could help. Might be good to discuss user interface design for this in an issue?

I've also added a parameters field to BoundaryFunction.
👍

@ali-ramadhan
Copy link
Member

Since this is a breaking change (?) do you want to bump to version 0.27.0 in this PR?

@glwagner
Copy link
Member Author

Since this is a breaking change (?) do you want to bump to version 0.27.0 in this PR?

Will do.

@glwagner
Copy link
Member Author

Ready to merge.

@glwagner
Copy link
Member Author

glwagner commented Mar 16, 2020

Since this is a breaking change (?) do you want to bump to version 0.27.0 in this PR?

Ah, it isn't a breaking change btw (it just adds a wrapper around existing functionality) --- but it does add a new feature, so 0.27.0 could still be warranted.

@glwagner glwagner merged commit 8e3c275 into master Mar 16, 2020
@glwagner glwagner deleted the glw/forced-flow-verification branch March 16, 2020 20:02
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 this pull request may close these issues.

3 participants