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

Adds halo argument to constructor to RegularCartesianGrid #508

Merged
merged 1 commit into from
Oct 28, 2019

Conversation

glwagner
Copy link
Member

Plus some cleanup of input validation, and a test.

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.

Might be nice for the user not to worry about setting halo sizes but we can worry about doing this more generally/automatically once we have other advection schemes.

@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #508 into master will decrease coverage by <.01%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
- Coverage   72.33%   72.32%   -0.01%     
==========================================
  Files          39       39              
  Lines        1717     1720       +3     
==========================================
+ Hits         1242     1244       +2     
- Misses        475      476       +1
Impacted Files Coverage Δ
src/grids.jl 79.24% <78.57%> (-0.76%) ⬇️

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 525d5c0...c2b4c1f. Read the comment docs.

@glwagner
Copy link
Member Author

glwagner commented Oct 28, 2019

Quite agree, though we will still need a constructor that allows us to set halos.

The design I am thinking of is that every term in the equations is associated with a function; something like

required_halo_size(::BiharmonicDiffusivity) = (2, 2, 2)

Then in the Model constructor (or Equation constructor, see #259) we can determine the required halo size for the grid based on the maximum required halo size for each term.

Each grid then needs a function

with_halo(halo, grid)

which returns a new grid that is identical to the old except for the addition of halos.

@glwagner glwagner merged commit 7d43ba8 into master Oct 28, 2019
@glwagner glwagner deleted the glw/settable-halo-size branch October 28, 2019 11:54
arcavaliere pushed a commit to arcavaliere/Oceananigans.jl that referenced this pull request Nov 6, 2019
Adds halo argument to constructor to `RegularCartesianGrid`

Former-commit-id: 7d43ba8
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.

None yet

2 participants