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

Why does SplitExplicitAuxiliaryFields need to be extended for TripolarGrid? #51

Open
glwagner opened this issue Nov 10, 2024 · 7 comments

Comments

@glwagner
Copy link
Member

These kinds of deep changes into Oceananigans mean that the interface for using different grids with the split-explicit free surface is not well defined. Either we need to have a well-defined interface that only extends fields by design (not simply ad hoc), or we need to move this functionality into Oceananigans

function SplitExplicitAuxiliaryFields(grid::TRG)

@glwagner
Copy link
Member Author

glwagner commented Nov 10, 2024

I think it is actually incorrect for "split explicit free surface" functionality to appear in a package that is supposedly only about grids in the first place. Everythign in this package should be purely about grids, not physics. The fact that we have to put physics and model-specific things in this package signals that our design is wrong.

@glwagner
Copy link
Member Author

@simone-silvestri
Copy link
Collaborator

simone-silvestri commented Nov 10, 2024

The estension here in done to extend the halos in the north region where the boundaries are special. We could move this to Oceananigans. But after CliMA/Oceananigans.jl#3894 it will not be needed anymore, only the materialize_free_surface function will need to be extended

@simone-silvestri
Copy link
Collaborator

In CliMA/Oceananigans.jl#3894 I can also modify the materialize_free_surface to automatically extend the halos for Connected topologies such that there is no need for extending anything here

@glwagner
Copy link
Member Author

In CliMA/Oceananigans.jl#3894 I can also modify the materialize_free_surface to automatically extend the halos for Connected topologies such that there is no need for extending anything here

Probably, you want to make this change regardless of whether we have a separate package for TripolarGrid or not

@glwagner
Copy link
Member Author

The other disadvantage of this as a separate package is that users now have to depend on OrthogonalSphericalShellGrids, which basically adds a line of boilerplate / dependency to every user environment

@glwagner
Copy link
Member Author

glwagner commented Nov 10, 2024

That said, it can well-motivated in two cases:

  1. This package does not actually depend on Oceananigans, and can be used outside of it (I think this is the case for SeawaterPolynomials for example)
  2. This package adds significant additional functionality that is not desired in Oceananigans. In that case, having it in a separate package will make loading / using Oceananigans more efficient, when the TripolarGrid is not needed.

I don't think that we meet either of these requirements though --- this package depends on Oceananigans and cannot be used separately from it. Also, this is a rather small package that provides a minor extension to Oceananigans and therefore I don't think significantly adds to Oceananigans bloat.

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

No branches or pull requests

2 participants