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

Migrate CA column ops to ClimaCore #1445

Merged
merged 10 commits into from
Oct 19, 2023
Merged

Migrate CA column ops to ClimaCore #1445

merged 10 commits into from
Oct 19, 2023

Conversation

charleskawczynski
Copy link
Member

This PR moves the column indefinite integrals and column mapreduce from ClimaAtmos to ClimaCore. These are needed for a few different parameterizations (e.g., radiation, gravity wave, turbconv etc.).

@dennisYatunin, I didn't see tests for this in ClimaAtmos, can you please take this PR over and add some tests?

@dennisYatunin
Copy link
Member

dennisYatunin commented Sep 26, 2023

I just made the following updates to this PR:

  • Add unit tests for column_integral_definite!, column_indefinite_integral!, and column_mapreduce! that check for correctness, type stability, and allocations. Run the tests on both CPUs and GPUs.
  • Use RecursiveApply in column_integral_definite! and column_indefinite_integral! to make them compatible with Tuples and NamedTuples. Check that RecursiveApply is used correctly in the unit tests.
  • Restrict column_integral_definite! to only accept cell-center fields as inputs because it gives incorrect results for cell-face fields. This is due to the fact that Δz_field(face_field) does not contain information about the half-widths of the topmost and bottommost cells. Only Fields.local_geometry_field(face_field).WJ contains this information, but we cannot use WJ to compute vertical integrals in 2D or 3D spaces because it is proportional to the cell volume, rather than the cell height. ClimaAtmos is currently only using column_integral_definite! on cell-center fields, so we can safely make this restriction.
  • Rewrite column_mapreduce! to reduce its allocations. Unfortunately, I was not able to completely get rid of the allocations, but I was able to bring them down. I think the remaining allocations have something to do with mapping a function over a tuple of fields (or, more precisely, a Vararg of fields). The function contains a captured variable (in this case, the level at which the fields are being accessed), so maybe the allocations are related to that.
  • Reformat column_integral_definite!, column_indefinite_integral!, and column_mapreduce! so that they have similar variable names, docstrings, etc.
  • Add the new docstrings to the Operators documentation page.

@charleskawczynski @simonbyrne Could you take a look at the tests and other changes?

@dennisYatunin
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Sep 26, 2023
@dennisYatunin dennisYatunin force-pushed the ck/column_ops branch 3 times, most recently from c84f716 to 9d21900 Compare September 26, 2023 21:34
@bors
Copy link
Contributor

bors bot commented Sep 26, 2023

try

Build failed:

@dennisYatunin
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Sep 26, 2023
@bors
Copy link
Contributor

bors bot commented Sep 27, 2023

try

Build failed:

@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Oct 11, 2023
@bors
Copy link
Contributor

bors bot commented Oct 11, 2023

try

Build failed:

@charleskawczynski charleskawczynski force-pushed the ck/column_ops branch 2 times, most recently from 245bb81 to 8b0d1f4 Compare October 11, 2023 16:25
@charleskawczynski
Copy link
Member Author

I've updated the names to match the existing ones. The tests look good to me, I think this is ready to merge.

@charleskawczynski
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Oct 11, 2023
1445: Migrate CA column ops to ClimaCore r=charleskawczynski a=charleskawczynski

This PR moves the column indefinite integrals and column mapreduce from ClimaAtmos to ClimaCore. These are needed for a few different parameterizations (e.g., radiation, gravity wave, turbconv etc.).

`@dennisYatunin,` I didn't see tests for this in ClimaAtmos, can you please take this PR over and add some tests?

Co-authored-by: Charles Kawczynski <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 11, 2023

Build failed:

bors bot added a commit that referenced this pull request Oct 14, 2023
@bors
Copy link
Contributor

bors bot commented Oct 14, 2023

try

Build failed:

@charleskawczynski
Copy link
Member Author

It turned out that the gpu tests were not exercised in the original commit, and some changes were needed. I got the column_mapreduce! working in the column case, but it's failing compilation on the gpu for the extruded space case.

That function is only currently needed / used in the old EDMF dycoms case, however, it's a very useful function and we should get it working at some point (e.g., it will come in handy in the gravity wave parameterization). So, I'm included to leave it as a broken test for now. I think this PR is ready to go, I'm happy to squash if that's preferred.

Copy link
Member

@dennisYatunin dennisYatunin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New changes look good to me!

@charleskawczynski
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Oct 18, 2023
1445: Migrate CA column ops to ClimaCore r=charleskawczynski a=charleskawczynski

This PR moves the column indefinite integrals and column mapreduce from ClimaAtmos to ClimaCore. These are needed for a few different parameterizations (e.g., radiation, gravity wave, turbconv etc.).

`@dennisYatunin,` I didn't see tests for this in ClimaAtmos, can you please take this PR over and add some tests?

Co-authored-by: Charles Kawczynski <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 19, 2023

Build failed:

@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Oct 19, 2023
@bors
Copy link
Contributor

bors bot commented Oct 19, 2023

try

Build failed:

@charleskawczynski
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 19, 2023

Build 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.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 9c22047 into main Oct 19, 2023
6 checks passed
@bors bors bot deleted the ck/column_ops branch October 19, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants