Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
D-grid data input method #42
D-grid data input method #42
Changes from 19 commits
e70894d
2a8b97a
e2cdd5c
b4d9e5d
7038e30
5b0991d
9001af1
65c0e14
dedd711
21e76c6
17e281b
3a627aa
f21774f
396025c
a63be71
d95e1fd
99b47ed
a536d95
6b9ff5e
8e0b829
caa2d43
6690e68
ada497b
6c1acfb
d48f2ba
136db1a
0be3439
2d8eb41
b7f379f
264549a
75a3e1d
f0a4a4d
20ffd3f
1b7ca1a
39467a9
f6f1bb5
f37c4fd
772e474
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This not part of the PR. This wasn't picked up by the tests because the 54 ranks tests are not running right now!
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 there way I can include them? Should we be running the 54 rank tests?
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.
How big are the files? Technically I think it should be located at
driver/examples/data
so that the example folder is indeed self standing.We should be running 54 ranks but I haven't look at if we can do that on the
Github
free runners without it taking 8 million years. Someone has to go ahead and make a github action and test it outThere 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 can start a plan to set up a CI which uses cloud resources from the NOAA cloud resources provider. We've done this successfully for SHiELD, FMS, and other GFDL repos. The key is to make sure we have a container with the needed bits.
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.
@FlorianDeconinck What do you think about including the tile data file in the PR? I know we don't love having test data in the repo but this one is small and if we want the tests run as part of CI...
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.
See above.
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.
Since these are small files, they should be okay. Ideally we'd store them in the container (mentioned above) or generate them on the fly to test the grid creation and ingest as a group.
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.
Ok so we are using the read-in area too?
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.
Yes, I figured since it was there to read it in as well, unless this is ill-advised
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's fine, just double-checking 👍
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.
IIRC, the fortran version merely read in the lat-lon pairs and calculated all other metrics. It would be valuable to know how the values in the files compare to those calculated by the model code from the lat-lon pairs.
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 could switch so that Pace behaves the same way and use dx, dy, and area for the tests
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 did discuss with Lucas whether we should carry the metrics in memory or recompute in the few instances where they are necessary. I know many of the metrics terms are for exclusively for remapping the winds amongst the various representations of A-, C-, and D-grid.
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.
So just reviewed the Fortran code and here's what I got for a few of the cases we care about...
cartesian grid
orthogonal grid
CS grid
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.
Many of the metrics are infrequently used or in specific situations that are not always necessary. I think it would be best for some architectures (especially GPUs) to compute on-the-fly, although for traditional CPUs it may still be best to compute metrics once and save them in memory.
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.
My 2 cent on an advanced system for this kind of "infrequent-but-vital" field.
Removing memory pressure on the GPU will quickly become a subject. Since the DSL generates from IR, in all backends, we can technically inject code. Therefore, we could have a system that registers this kind of calculation and at JIT compilation time, when we known about the size of the domain and can do memory pressure projection (which already exists in DaCe backend), we could decide to flip the read from those field by the analytics.
Of course this has a few caveats, especially that it can be prohibitive if the analytic solution calls on other fields that themselves can be inlined.
Ultimately, this kind of decision (save or recompute) should be taken of the hands of the modelers and push down the stack.