-
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
Allow users to specify the domain when creating a `RegularCartes… #464
Conversation
While we are breaking the API, why not use RegularCartesianGrid(Float64; size=(10, 10, 10), x=(-5, 5), y=(-π, π), z=(0, 1)) It's not only more verbose but also removes the sometimes painful fact that we really want to use I'd also propose adding a constructor of the form RegularCartesianGrid(FT, nx, ny, nz; length) = RegularCartesianGrid(FT; size=(nx, ny, nz),
x=(-length[1]/2, length[1]/2), y=(-length[2]/2, length[2]/2), z=(-length[3], 0)) This has a syntax similar to the RegularCartesianGrid(FT, size::Tuple; length) = RegularCartesianGrid(FT, size...; length) (Along with the corresponding constructors with default float types.) I'm not too fond of We've also never discussed this --- but why do we use Edit: if |
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.
I like this and think its a positive change. Could be good to add a few more small niceties. Major point of discussion is N
versus size
.
I think it's a good idea to merge a
Now that
I'm on board with a change to lowercase
Same here, it's a legacy constructor but we can get rid of it in this PR which would also help address #429.
This seems like a good default. |
On that note I feel that I wonder if it's better to define abstract types like Then you can dispatch on e.g. |
What information do you need for a stretched grid? Is this just a matter of In that case we just have Edit: may be best to save this change until we have a stretched grid. We haven't really discussed how we'd construct stretched grids, but our design may depend on that. |
True, good idea. My main concern was readability as we dispatch on |
Unfortunately can't dispatch on keyword arguments julia> f(x; y, z) = x + y + z
f (generic function with 1 method)
julia> f(x; n) = f(x; y=n, z=2n)
f (generic function with 1 method) so I don't think we can use this constructor RegularCartesianGrid(FT, nx, ny, nz; length) = RegularCartesianGrid(FT; size=(nx, ny, nz),
x=(-length[1]/2, length[1]/2), y=(-length[2]/2, length[2]/2), z=(-length[3], 0)) but maybe we can define a Or we can define an extra keyword argument RegularCartesianGrid(FT; size, length=nothing, x=nothing, y=nothing, z=nothing) so you either specify a Also, do we want lowercase |
best to avoid lowercase I thought |
Adding |
to use new `RegularCartesianGrid` constructor.
RegularCartesianGrid
RegularCartesianGrid
I went a bit further and nuked the legacy constructors so that there's only one now: RegularCartesianGrid(FT=Float64; size, length, x, y, z) Again, this is a breaking API change although I think we should be nuking legacy constructors now while the code is not being widely used (#429). I'd like to do something similar for |
RegularCartesianGrid
RegularCartesianGrid
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.
Examples need to be fixed.
Curious what @suyashbire1 thinks. This is a little late but it is fairly annoying that size
and length
are the name of base functions and somehow it just doesn't ring right for me (can't put my finger on it). We could use resolution
and extent
or something like that (in hindsight those are typically the words I'd use in a scientific paper...)
Might be good to put out an informal poll before making this change to see what people find most intuitive.
Bleh.
src/grids.jl
Outdated
Ax::T | ||
Ay::T | ||
Az::T | ||
Ax::FT |
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.
We don't need areas and volumes right?
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.
That's true, we currently don't use them.
But thinking about it a bit more, I think we might want to start using them in PR #283. With the more general finite volume operators, we have areas that are computed like
@inline Az(i, j, k, grid) = Δx(i, j, k, grid) * Δy(i, j, k, grid)
although for RegularCartesianGrid
and VerticallyStretchedCartesianGrid
it might make more sense to use
@inline Az(i, j, k, grid) = grid.Az
to avoid an extra multiplication at the very lowest level.
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.
Not sure you'd see that benefit since I think our costs are dominated by accessing memory. But could be worth benchmarking. If it has no cost I think doing the multiplication is fine; makes the structs simpler which is well worth it. "Store information in functions".
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.
That's a good point. I'll remove them here as they're currently not being used. And I'll benchmark the two options when I work on merging in #283.
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.
It might make more sense for vertically stretched grids where grid.ΔzF[k]
involves an extra memory access but we'll find out when we benchmark the vertically stretched grid.
# Hack that allows us to use `size` and `length` as keyword arguments but then also | ||
# use the `size` and `length` functions. | ||
sz, len = size, length | ||
length = Base.length |
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.
Hmm, yes, that's annoying.
Codecov Report
@@ Coverage Diff @@
## master #464 +/- ##
==========================================
- Coverage 73.34% 73.31% -0.04%
==========================================
Files 27 27
Lines 1508 1525 +17
==========================================
+ Hits 1106 1118 +12
- Misses 402 407 +5
Continue to review full report at Codecov.
|
Yeah it's not perfect, might be good to discuss a bit so we can merge in a grid we're all happy with (might as well make all breaking changes right now).
Fixed them and example tests pass on my laptop. But yeah this might be a problem in the future as we're skipping them...
I agree. One bonus point is that Hmmm, I'm not opposed to |
Both @christophernhill and @suyashbire1 vote for |
Awesome. Do we want to make any more changes to |
RegularCartesianGrid
So now we can specify the domain using
but if we only care about the length of each dimension then you can still use
This is nice for a lot of problems where the domain isn't
x=(0, Lx), y=(0, Ly), z=(-Lz, 0)
.I also learned that keyword arguments do not participate in dispatch so you can't have both
RegularCartesianGrid(T; N, L)
andRegularCartesianGrid(T; x, y, z)
:( Apparently it's a pretty old Julia issue: JuliaLang/julia#9498Resolves #413