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

Ability to construct field tuples with non-zero data #627

Merged
merged 15 commits into from
Feb 19, 2020

Conversation

ali-ramadhan
Copy link
Member

This PR is part 1/3 of making boundary conditions a field property.

I was refactoring the fields module, and in doing so I added optional kwargs to allow for data arrays to be passed.

This allows the checkpointer to be simplified as it no longer has to keep track of array references, and makes restoring/checkpointing large models possible. I will address this in part 2.

Changes:

  1. AbstractLocatedField has been removed. Now there is only AbstractField.

  2. Fields can now be initialized with a data array. This will make restoring/checkpointing large models possible.

    Right now field.data can store anything. We could check to make sure that field.data has the same size as the grid, but this is extra code and could limit flexibility in the future so I did not add a check for this.

  3. Field tuple initializers (VelocityFields, TracerFields, etc.) can now accept data arrays through keyword arguments.

  4. FaceFieldX has been renamed to XFaceField, and same for y and z.

Note: This PR branches off #626.

@ali-ramadhan ali-ramadhan added the cleanup 🧹 Paying off technical debt label Feb 16, 2020
@codecov
Copy link

codecov bot commented Feb 16, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@c417701). Click here to learn what that means.
The diff coverage is 84.84%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #627   +/-   ##
=========================================
  Coverage          ?   75.13%           
=========================================
  Files             ?      118           
  Lines             ?     2280           
  Branches          ?        0           
=========================================
  Hits              ?     1713           
  Misses            ?      567           
  Partials          ?        0
Impacted Files Coverage Δ
src/Oceananigans.jl 100% <ø> (ø)
src/Models/show_models.jl 0% <ø> (ø)
src/Architectures.jl 88.88% <ø> (ø)
src/Fields/Fields.jl 100% <ø> (ø)
src/Fields/show_fields.jl 0% <0%> (ø)
src/Simulations.jl 93.75% <100%> (ø)
src/Fields/field.jl 78.78% <88.88%> (ø)
src/Fields/field_tuples.jl 84.21% <90.9%> (ø)

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 c417701...9187570. Read the comment docs.

@glwagner
Copy link
Member

Couple quick questions:

  1. It was already possible to initialize fields with an array (otherwise we could not construct them). What specifically does this contribution change, and why is it important? There is a distinction between raw arrays and offset arrays. Is this what the PR description refers to?

  2. We can add a validate_data check that only errors when we know data is incorrect, and otherwise does not fuss for, eg, non-array types.

@ali-ramadhan ali-ramadhan changed the title Ability to construct fields with non-zero data Ability to construct field tuple with non-zero data Feb 17, 2020
@ali-ramadhan ali-ramadhan changed the title Ability to construct field tuple with non-zero data Ability to construct field tuples with non-zero data Feb 17, 2020
@ali-ramadhan
Copy link
Member Author

It was already possible to initialize fields with an array (otherwise we could not construct them). What specifically does this contribution change, and why is it important? There is a distinction between raw arrays and offset arrays. Is this what the PR description refers to?

Sorry I meant that we can now initialize field tuples (VelocityFields, TracerFields, etc.) with non-zero data. I updated the title. This change was important because it's required for checkpointing/restoring large model (implemented in #628). The field tuple constructors are further generalized in #631.

We can add a validate_data check that only errors when we know data is incorrect, and otherwise does not fuss for, eg, non-array types.

Seems like a good idea. fill_halo_regions! assumes field.data has a certain size consistent with the size of the grid. We can just check that size(field.data) == size(grid) plus halo regions.

@glwagner
Copy link
Member

Sorry I meant that we can now initialize field tuples (VelocityFields, TracerFields, etc.) with non-zero data. I updated the title. This change was important because it's required for checkpointing/restoring large model (implemented in #628). The field tuple constructors are further generalized in #631.

Sounds good. Just for the record, model.velocities is just a named tuple, so if individual fields can be manually built with pre-allocated data, then so can the named tuples that hold velocity field data and tracer data. I suppose the change here is a function that achieves the same. If the current changes make the checkpointer code clearer, than I am for it.

fill_halo_regions! assumes field.data has a certain size consistent with the size of the grid.

Let's be specific here. fill_halo_regions! does not assume anything about the "size" of data. Specifically, it invokes getindex on the property parent of .data at certain indices. Therefore such a method call must work for .data. In the case that .data is either a plain Array or a CuArray, we know from the properties of those types when such a getindex call for certain indicies will fail. Therefore, we should feel free to intercept such an error during Field construction. In other cases we cannot be sure that such a method will fail because we don't know how getindex works on .parent for arbitrary types. In the spirit of "not assuming what we don't know", we should opt to throw errors only in cases that we know an error should occur.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

Lots of changes to wade through here so its hard to review in detail. But since tests pass and I support the spirit of this change I am approving.

@@ -1,62 +1,64 @@
"""
VelocityFields(arch, grid)
VelocityFields(arch, grid; u=zeros(arch, grid), v=zeros(arch, grid), w=zeros(arch, grid))
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be useful to users who want to partially-initialize velocities with a predefined field for one of u, v w, so I'm all for it.

@ali-ramadhan
Copy link
Member Author

Therefore, we should feel free to intercept such an error during Field construction. In other cases we cannot be sure that such a method will fail because we don't know how getindex works on .parent for arbitrary types. In the spirit of "not assuming what we don't know", we should opt to throw errors only in cases that we know an error should occur.

Good point, I'll add a simple check for this during field creation.

@ali-ramadhan ali-ramadhan merged commit 4ef4d11 into master Feb 19, 2020
@ali-ramadhan ali-ramadhan deleted the ar/bcs-in-ur-fields branch February 19, 2020 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abstractions 🎨 Whatever that means cleanup 🧹 Paying off technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants