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

Field major refactor: one Field to rule them all #2121

Merged
merged 150 commits into from
Jan 15, 2022
Merged

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Dec 15, 2021

This PR resolves #2052 by consolidating Field to encompass:

  • Field
  • ComputedField
  • ReducedField
  • AveragedField

In addition, it gets rid of KernelComputedField since that functionality is now covered by KernelComputedOperation + Field.

Field will no longer explicitly keep track of architecture, because architecture belongs to grid now.

This PR also change the API for constructing fields:

a = Field{Face, Face, Center}(grid) # constructs a field at `(Face, Face, Center)`
b = Field{Nothing, Face, Center}(grid) # constructs a field that is reduced in the x-direction at `(Face, Center)` in `y, z`
c = Field{Center, Center, Center}(grid)
csq = Field(c^2) # constructs a "computed field" that calculates and stores c^2 via `compute!(c_sq)`

There are also convenience constructors that take a tuple of locations:

loc = (Face, Face, Center)
a = Field(loc, grid)

AveragedField functionality is now wrapped into Field. Rather than writing c_sq_avg = AveragedField(c^2, dims=(1, 2)), we now write

c_sq_avg = Field(Average(c^2, dims=(1, 2)))

There is also an interface for generic reductions, so another possibility is

c_sq_max = Field(Reduction(maximum!, c^2, dims=(1, 2)))
compute!(c_sq_max)

It was already possible to compute maximum! with a ReducedField and an AbstractField; this interface simply provides a way to compute these things on the fly for diagnostics purposes while running simulations.

In summary, this PR is meant to reduce the amount of code we have to maintain, significantly declutter our Field implementations, clean up the API, and hopefully make it easier to extend Field implementations in the future, especially regarding reductions of operations such as SummedField, SlicedField, etc.

@glwagner glwagner marked this pull request as draft December 15, 2021 16:55
@francispoulin
Copy link
Collaborator

This sounds like a great idea, and a big step forward.

One question: Considering that we use Flat in Grid to specify when a dimension is not to be built, could we use Flat instead of Nothing to signify that in this PR?

@glwagner
Copy link
Member Author

This sounds like a great idea, and a big step forward.

One question: Considering that we use Flat in Grid to specify when a dimension is not to be built, could we use Flat instead of Nothing to signify that in this PR?

Well, we distinguish between topologies (Periodic, Bounded, Flat) and locations (Center, Face, Nothing).

Are you suggesting that we introduce a new location called Flat, the same name as the topology Flat?

@francispoulin
Copy link
Collaborator

No, sorry, my silly mistake. Nothing makes a lot of sense. Please ignore my previous comment.

@glwagner
Copy link
Member Author

glwagner commented Dec 15, 2021

No, sorry, my silly mistake. Nothing makes a lot of sense. Please ignore my previous comment.

It still might make sense to come up with a better location than Nothing... (I guess the meaning is that in a reduced or Flat direction, a field has "no location").

@francispoulin
Copy link
Collaborator

How about None? A bit shorter than NoLocation.

@navidcy navidcy changed the title One Field to rule them all [WIP] Field major refactor: one Field to rule them all [WIP] Dec 16, 2021
@glwagner glwagner changed the title Field major refactor: one Field to rule them all [WIP] Field major refactor: one Field to rule them all Dec 31, 2021
@glwagner glwagner marked this pull request as ready for review December 31, 2021 16:52
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.

Coalescing Field definitions
4 participants