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

Coalescing Field definitions #2052

Closed
glwagner opened this issue Nov 11, 2021 · 1 comment · Fixed by #2121
Closed

Coalescing Field definitions #2052

glwagner opened this issue Nov 11, 2021 · 1 comment · Fixed by #2121
Labels

Comments

@glwagner
Copy link
Member

glwagner commented Nov 11, 2021

It's been pointed out several times (most recently by @sandreza) that the many different Field types are potentially redundant, or that we might get away with one 'general' definition.

One general definition might be something like

struct Field{X, Y, Z, ...}
    data
    grid
    architecture
    boundary_conditions
    operand
    status
end

more or less mirroring ComputedField. We then have the following equivalencies to existing data structures:

  • isnothing(operand) and isnothing(status) recovers Field
  • operand isa AbstractOperation recovers ComputedField
  • one or more of X, Y, Z is Nothing implies the field is reduced along the Nothing dimension.

For "computed reductions", we can define new operand wrappers like Averaged and Integrated with dims properties (maybe Average, or tense other than past or present, is better).

We'll probably want to retain KernelComputedField and FunctionField.

This would reduce the number of structs we define significantly and might otherwise result in a significant reduction of code. It also clarifies how custom computed fields are defined: basically we might have something like

compute!(field::Field) = compute!(field, field.operand)

This falls back to a no-op when operand::Nothing, but supports other behavior is operand::AbstractOperation, operand::Average{<:AbstractOperation}, or some other user-defined type.

We can keep the existing user interface if we want, or we can change it. I'm less sure about what's best there. We probably wouldn't have a use for ComputedField, but for averages we could have

avg_c = Field(Average(c, dims=1))

or

avg_c = Field(c, Average(dims=1))

or, as before,

avg_c = AveragedField(c, dims=1)

The last seems like the most readable, but obfuscates the source since there'd no longer be a struct AveragedField (this is one example of a pervasive problem in the source, but that's a topic for another issue).

Note that previously this was deemed difficult because we typically throw away locations when adapting Field to the GPU (see eg #746), which means that the above solution might break broadcasting with reduced fields. But I only realized (duh...) that we can easily add special adapt_structure methods for the case that some locations are reduced, which solves the problem.

@simone-silvestri
Copy link
Collaborator

I would rather the first option (avg_c = Field(Average(c, dims=1))) because it avoids to have to code functions as IntegrateField .... and I think option option 1 is clearer than option 2 (you see that the average operation is applied to the fields c)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants