-
Notifications
You must be signed in to change notification settings - Fork 191
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
Grids now have a topology #614
Conversation
Codecov Report
@@ Coverage Diff @@
## master #614 +/- ##
==========================================
+ Coverage 74.55% 74.59% +0.03%
==========================================
Files 117 118 +1
Lines 2209 2220 +11
==========================================
+ Hits 1647 1656 +9
- Misses 562 564 +2
Continue to review full report at Codecov.
|
I don’t like “Singleton”: Periodic and Bounded refer to the physical nature of the dimension, while Singleton obliquely references the size of an underlying array of discrete data. A better name would refer to the physical nature of the coordinate; “Uniform” or “Flat” are examples. It could make sense to make (Periodic, Periodic, Bounded) a default. This is already the default of Model. |
We're setting up a discrete model anyways, but I guess if it's supposed to model some physical system then "Singleton" may not be completely appropriate. I'd vote for Flat over Uniform then. Uniform could be multiple grid points while Flat strongly implies one grid point (while both imply homogenous dynamics in that dimension).
Yeah I guess we have a choice here: do we want to make horizontally periodic the default or are users now required to specify a grid topology? I could see benefits either way. Keeping it the default would introduce no breaking changes and simplify the tests, but requiring it would improve the clarity of most scripts (an issue that has been brought up before, see #459). I think most real scripts (not tests) should be specifying a topology for clarity. |
Another design we can consider is something like: grid = RegularCartesianGrid(size=(x=128, z=64), x=(-1, 1), z=(-1, 0), topology=(x=Periodic, z=Bounded)) where since This makes the grid constructors more verbose but has the added benefit of being clearer and not having to specify flat dimensions, i.e. it's an xz-model so y should not even be mentioned. It would require a lot more refactoring though... but now's the time I guess. |
I think explicit is better. Why do we need to refactor? Why don't we just specify a default topology in the grid constructor? Then, the only refactoring we need to do is for models in the channel domain. |
I'm fine with |
I'll add a default The bigger refactor would be if we decided to go with named tuples like
True but I don't see the point of running with more than one grid point in a dimension that's supposed to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great.
Oceananigans.Operators | ||
|
||
using Oceananigans: AbstractGrid | ||
using Printf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this preferred style? Why? I like fewer characters...
using Oceananigans.Architectures: @hascuda | ||
@hascuda using CUDAnative, CuArrays | ||
|
||
using Oceananigans.Architectures | ||
using Oceananigans.Grids | ||
using Oceananigans.Operators | ||
using Oceananigans.Coriolis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicit imports might be nice here for all these physics terms... but we can save this for later.
This PR adds grid topologies:
Periodic
,Bounded
, andSingleton
. We should finalize our choice of named before merging.As
topology
is now a required kwarg of all grid constructors tons of refactoring was needed. It also does a bit of cleanup:AbstractGrid
is now defined in the Grids submodule, and grids no longer have theTx, Ty, Tz
property (total number of grid points).Documentation should be updated before this PR is merged.
Resolves #446
Resolves #459
Resolves #489