-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add Grids, make them mutable objects #1487
Conversation
With regards to integration in I also defined some |
@Sbozzolo for ClimaAtmos, could we just use the ClimaCore grid objects directly? |
I think we should have simple and physically-relevant objects in In
For 2 of these 4, we want to support topography and optionally enable the bubble correction. These are clearly distinct |
We could probably define type aliases that would allow you to do dispatch, e.g.
|
If it would help, we could make this simpler by making the lower level objects (domain, mesh) fields of the grid as well? |
bors try |
tryBuild failed: |
tryBuild failed: |
9e15a8f
to
2ddd02d
Compare
bors try |
tryBuild succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Okay, we're passing here and on ClimaAtmos: https://buildkite.com/clima/climaatmos-ci/builds/14490#018b76ad-a266-4930-afb5-7d104496ae6b |
face_local_geometry::LG | ||
end | ||
|
||
@memoize WeakValueDict function ExtrudedFiniteDifferenceGrid( |
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.
Can we document (or provide a link to docs to show) what @memoise WeakValueDict
is doing here? or somewhere?
@@ -50,7 +50,7 @@ single_field_solve!(::ClimaComms.AbstractCPUDevice, cache, x, A, b) = | |||
_single_field_solve!(cache, x, A, b) | |||
function single_field_solve!(::ClimaComms.CUDADevice, cache, x, A, b) | |||
Ni, Nj, _, _, Nh = size(Fields.field_values(A)) | |||
nthreads, nblocks = Spaces._configure_threadblock(Ni * Nj * Nh) | |||
nthreads, nblocks = Topologies._configure_threadblock(Ni * Nj * Nh) |
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.
Can/should we instead move _configure_threadblock
to a separate file or something?_configure_threadblock
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.
what do you mean?
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.
_configure_threadblock
doesn't really have anything to do with Topologies
Co-authored-by: Charles Kawczynski <[email protected]>
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.
Overall, the changes look good, and I want to slow this down given the quality of life improvement it brings!
And thanks for testing in CliMA Atmos! |
Fixes #1120 and #1467.
I've gone with option 2 in #1120 (comment).
In the following benchmark:
On main:
on this branch