Oceananigans.jl icon indicating copy to clipboard operation
Oceananigans.jl copied to clipboard

Creation of `FieldTimeSeries` from `BinaryOperation` of other FTS + some FTS bug fixes

Open jagoosw opened this issue 1 year ago • 4 comments

This PR primarily adds a method to FieldTimeSeries to build them from operations on other FTS similar to what can be done with Field. For example:

u10 = FieldTimeSeries("atmosphere.jld2", "u10")
Q   = FieldTimeSeries(- ρₐ / ρₒ * cᴰ * u10 * abs(u10))

This does work in a different way to e.g. Field(- ρₐ / ρₒ * cᴰ * u10 * abs(u10)) where u10 is instead a field, because FTS don't have operand properties so instead we're filling the data of the FTS when it is made (which is obviously very memory inefficient for big FTS).

I have also added some methods for getindex on FTS with a flat dimension and Time indexing as they don't exist like they do for 4D FTS, and some methods for @at to work on flattened fields.

jagoosw avatar Jun 30 '24 10:06 jagoosw

Interesting. We have deliberately avoided operations for field time series but we can consider it. How does this work for operations that involve both a Field and a FieldTimeSeries? I'm worried this will be a bit brittle and potentially lead to some wasted time for users, since it may only work for the simplest cases. Another issue is that we don't have a time derivative operator.

Another way to implement a time series computation is to make a new/empty FieldTimeSeries, and then loop over the time-index to compute each field in the time series. Is this PR merely a convenience to avoid that loop? Are there other solutions / syntax we could think about supporting that might be more useful / generalize better for users?

Note I think for big feature changes like this it's best to discuss in an issue first before having a PR, but we can discuss here.

glwagner avatar Jun 30 '24 17:06 glwagner

because FTS don't have operand properties

What's the rationale behind this implementation vs adding operand as a property to FieldTimeSeries? What's the long term vision for abstract operations with FieldTimeSeries?

glwagner avatar Jul 01 '24 12:07 glwagner

That all makes sense, I hadn't considered how big a change that was.

This is just a convenience constructor for that alternative so probably isn't the best solution (and I checked it doesn't work when the operation is between both fields and fts).

It probably makes sense to add operators to FTS properly, and possibly warn users that operators don't work properly for FTS too? For I'll remove that code from this PR and just keep the bug fixes (which just make it possible to use a flattened FTS as a boundary condition).

jagoosw avatar Jul 01 '24 13:07 jagoosw

My main concern is that if it doesn't work for most cases (only simple ones) then we won't really want to document / advertise the new feature.

Right now this is what users have to do I think:

u10 = FieldTimeSeries("atmosphere.jld2", "u10")

tau = FieldTimeSeries{Face, Center, Nothing}(grid, u10.times)

for n = 1:length(tau)
    u10n = u10[n]
    tau[n] .=  - ρₐ / ρₒ * cᴰ * u10n * abs(u10n)
end

collapsing this to fewer lines and also potentially making it faster (by launching a 4D kernel that computes over time as well as space could be a significant advantage but I think we probably need to be able to mix Fields into the computation)

glwagner avatar Jul 01 '24 15:07 glwagner