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

What is the purpose of the 'dim' property of RegularCartesianGrid? #287

Closed
glwagner opened this issue Jun 13, 2019 · 6 comments
Closed

What is the purpose of the 'dim' property of RegularCartesianGrid? #287

glwagner opened this issue Jun 13, 2019 · 6 comments
Labels
cleanup 🧹 Paying off technical debt

Comments

@glwagner
Copy link
Member

The dim property:

https://github.com/climate-machine/Oceananigans.jl/blob/40dbd96fd45a30867063c476b3eecbe13db1fb5c/src/grids.jl#L12

appears to somehow indicate how many of the dimensions of RegularCartesianGrid are greater than 1... ?

I'm just not sure where this is used. It's also ambiguous, because a grid could be 2D or 1D in many different ways, and therefore I'm not sure what it's useful for. There seem to be a lot of lines in the grid constructor devoted to it.

@ali-ramadhan ali-ramadhan added the cleanup 🧹 Paying off technical debt label Jun 13, 2019
@ali-ramadhan
Copy link
Member

Yeah I agree it might not be a useful metric as it doesn't tell you which dimensions are flat.

I was going to use it to implement #35 and maybe #36 but these issues are low priority and there are extra halo and GPU complications so I'd be in favor of getting rid of the dim property for brevity.

@glwagner
Copy link
Member Author

glwagner commented Jun 13, 2019

How does it help with #35 and #36? It doesn't seem like you're able to infer which dimensions are flat or which terms to turn off based on dim.

To do multiple dispatch, this kind of property needs to be encoded with a type (or rather, a tuple or tuple-like collection of types for each of x, y, and z), not a number (unless you intend to call Val on every operation).

@ali-ramadhan
Copy link
Member

It doesn't, that's why I said it's not a useful metric. The only place it's used is in the show(io::IO, g::RegularCartesianGrid) function (pretty printing for grids) but it also prints Nx, Ny, Nz which tells you more information than dim.

I haven't given #35 much thought as it's low priority so not sure how to best approach it, but yeah probably some sort of multiple dispatch on which dimension is flat/not-flat. Seems like it would be pretty invasive and complicate the kernels so maybe best left until we really need that feature.

@glwagner
Copy link
Member Author

glwagner commented Jun 13, 2019

For #35 and #36 you can implement dispatch based on the type (or a type parameter) of the Grid type. Using multiple dispatch is non-invasive.

Dispatch on values (like dim) is more invasive.

@ali-ramadhan
Copy link
Member

Yeah I guess the better approach would be to write multiple kernels which would increase the code size. Or implement multiple equations taking the approach of #259.

@glwagner
Copy link
Member Author

Closing this since its essentially redundant with #330.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup 🧹 Paying off technical debt
Projects
None yet
Development

No branches or pull requests

2 participants