CliMA / ClimaOcean.jl

🌎 Framework for realistic regional-to-global ocean simulations, and coupled ocean + sea-ice simulations based on Oceananigans and ClimaSeaIce. Basis for the ocean and sea-ice component of CliMA's Earth system model.
https://clima.github.io/ClimaOceanDocumentation/dev/
MIT License
30 stars 9 forks source link

`ECCORestoring` can be used directly as a forcing #177

Closed glwagner closed 2 days ago

glwagner commented 2 months ago

Right now, ECCORestoring is abstract as the forcing "function", which must then be further wrapped in Forcing by ECCO_restoring_forcing:

https://github.com/CliMA/ClimaOcean.jl/blob/b3b44b709c70b296adb63cf7b2f4552334dcfd1b/src/DataWrangling/ecco_restoring.jl#L276-L279

There's no barrier to using ECCORestoring directly as a forcing though, provided the appropriate functionality within Oceananigans is extended correctly. And this would be much better, not just for syntax but also because the implementation will be less complicated and easier to understand.

glwagner commented 2 months ago

A few other things:

ECCORestoring should use the rate of restoring as input, not timescale. This brings it inline with how Oceanangians.Forcing.Relaxation is used --- we want to minimize the cognitive dissonance between Oceananigans and ClimaOcean.

struct Relaxation{R, M, T}
      rate :: R
      mask :: M
    target :: T
end

I also think the property names can be improved. Probably data instead of ecco_fts and grid instead of ecco_grid.

https://github.com/CliMA/ClimaOcean.jl/blob/b3b44b709c70b296adb63cf7b2f4552334dcfd1b/src/DataWrangling/ecco_restoring.jl#L179-L185

Note that variable_name isn't even a name, its a type:

https://github.com/CliMA/ClimaOcean.jl/blob/b3b44b709c70b296adb63cf7b2f4552334dcfd1b/src/DataWrangling/ecco_restoring.jl#L183

This is also rather surprising to read:

https://github.com/CliMA/ClimaOcean.jl/blob/b3b44b709c70b296adb63cf7b2f4552334dcfd1b/src/DataWrangling/ecco_restoring.jl#L201

until you realize that getindex has been extended... but it's not an expected or usual pattern for getindex...

glwagner commented 2 months ago

Also what is the purpose of oceananigans_fieldname:

https://github.com/CliMA/ClimaOcean.jl/blob/b3b44b709c70b296adb63cf7b2f4552334dcfd1b/src/DataWrangling/ecco_restoring.jl#L156-L160

@simone-silvestri

glwagner commented 2 months ago

I guess in Oceananigans we decided to call this kind of thing Relaxation. If we prefer Restoring I think that's ok... we should change it in Oceananigans too though.

simone-silvestri commented 1 month ago

Also what is the purpose of oceananigans_fieldname:

https://github.com/CliMA/ClimaOcean.jl/blob/b3b44b709c70b296adb63cf7b2f4552334dcfd1b/src/DataWrangling/ecco_restoring.jl#L156-L160

@simone-silvestri

The purpose of having a type is that symbols and strings are not isbits so they cannot be passed to the GPU. Originally, I thought of having indices that specify the field that we need to retrieve, but that requires us to assume the position of model fields, so I think a new type with the variable name is cleaner.

glwagner commented 1 month ago

Right! But should we have users invoke the types directly? Generally its nice for the user interface to be as close as possible to the source code. It could get out of hand if there are lots of fields in the future. Also if you need to do something like convert units, it's useful to use Temperature(Kelvin()) which is not possible when using :temperature