EAMxx: hide field utils impl using a type-agnostic scalar wrapper #7857

bartgol · 2025-11-01T06:35:15Z

This design pattern uses a type-agnostic wrapper for scalars, so that we don't need to expose the templated method implementation to the customers.

[non-BFB] For runs using the IC perturb feature

A more detailed summary of changes

ScalarWrapper: tiny struct that stores a scalar as a double, but allows to retrieve it with any scalar type. This is almost as using double everywhere, but it adds some type safety, as it ensures that no narrowing happens when getting the scalar back. E.g., storing an int in a ScalarWrapper allows to later get it back with any type (no narrowing for int->float/double), but storing a float would cause an error if later used to retrieve the scalar with int type.
split impl of field utils into separate files, to speed up compilation
split some field utils unit tests into separate execs
for compute_mask, make the comparison type a runtime param. We don't use this utility often, and probably the branch predictor can already help to pick the right case anyways
for the horiz/vert contractions, remove the scalar type template arg and make AVG a runtime arg
make a few versions of the randomize util, using uniform, normal, or sampling distributions. The pdf params are passed via ScalarWrapper, and the engine is created internally, so that the API is non-templated.
simplify a bit the perturb api, avoiding template args and passing just the perturb magnitude.

For utils accepting ScalarWrapper, we also offer a templated version with the template arg being the scalar. Wait, isn't this undoing the whole idea of not having to template the utils? Well, the templated version calls the version that accepts ScalarWrapper, which is NOT templated, so that the true (long) impl of the util can still be hidden in the cpp file.

If this is merged, I would like to take a similar approach also for the field update methods (maybe even turning them into another set of field utils, rather than field methods).

Useful to pass around scalars without having to template every function on the passed scalar

Start from utils that are not templated

Use ScalarWrapper to hide implementation

Copilot

Pull Request Overview

This PR refactors the field utility APIs to simplify template-based interfaces and improve code organization. The key change is introducing a ScalarWrapper type to eliminate the need for templated scalar types in function signatures, allowing implementations to be moved from header files to compilation units.

Introduces ScalarWrapper class to wrap scalar values with type safety
Replaces templated scalar parameters with ScalarWrapper in field utility functions (randomize, perturb, field_min/max/sum, frobenius_norm, compute_mask, etc.)
Refactors field utility implementations into separate .cpp files for better compile-time performance
Updates randomize API to use seed-based interface instead of engine+PDF
Simplifies test code by using simpler randomization and utility function calls

Reviewed Changes

Copilot reviewed 66 out of 66 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`eamxx_scalar_wrapper.hpp`	New wrapper class for type-safe scalar value passing without templates
`eamxx_data_type.hpp`	Made functions device-compatible with `KOKKOS_INLINE_FUNCTION`
`field_utils.hpp`	Updated API signatures to use `ScalarWrapper` instead of templates
`field_utils_impl.hpp`	Deleted - implementations moved to separate `.cpp` files
`field/utils/*.cpp`	New files containing implementations split by functionality
`field.hpp/cpp`	Added optional `allocate` parameter to Field constructor
Test files	Updated to use simplified randomization and utility APIs

Comments suppressed due to low confidence (1)

components/eamxx/src/share/field/utils/randomize.cpp:1

In the randomize_discrete function at line 151 in coarsening_remapper_tests.cpp, seed++ is used without storing the result. This increments the seed but doesn't pass the incremented value to the function. The pattern should use seed and then increment separately, or use pre-increment. Same issue appears in multiple test files.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-01T11:43:46Z

components/eamxx/src/share/util/eamxx_data_type.hpp

+    EKAT_KERNEL_ERROR_MSG ("Error! Unsupported data type.\n");
+    return DataType::Invalid;


The return statement on line 38 is unreachable since EKAT_KERNEL_ERROR_MSG likely terminates execution. This return is only present to satisfy the compiler. Consider adding a comment to clarify this is unreachable code required for compilation.

components/eamxx/src/dynamics/homme/tests/dyn_grid_io.cpp

Copilot · 2025-11-01T11:43:47Z

components/eamxx/src/share/field/utils/horiz_contraction.cpp

+      impl::horiz_contraction<double>(f_out,f_in,weight,AVG,comm,f_tmp);
+      break;
+    default:
+      EKAT_ERROR_MSG ("[vert_contraction] Error! Unsupported data type.\n");


Error message says '[vert_contraction]' but this is in the horiz_contraction.cpp file. The error message should say '[horiz_contraction]' instead.

mahf708

This looks fine to me from a quick scan, I think it helps.

Quick question re:

If this is merged, I would like to

were you expected push back? Is there something specific you want us to look very closely at?

bartgol · 2025-11-03T20:36:45Z

Well, I don't want to assume my PRs will be for sure merged. I thought the design helps, but testing may reveal some underlying insormountable issue, or other ppl may have different opinions... E.g., someone may find the ScalarWrapper approach a bit obscure.

Use ScalarWrapper to hide the template implementation. The randomize util will ALWAYS use a uniform distribution. Use ranodmize_normal for a normal distribution

mahf708 · 2025-11-03T22:06:23Z

Sounds good. To me, it appears helpful. I will take a more thorough look soon, but I am approving based on a quick look.

Btw sorry, didn't mean to edit the reviewer section when I added copilot. It was a mistake.

Copilot

Pull Request Overview

Copilot reviewed 66 out of 66 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

components/eamxx/src/share/field/utils/views_are_equal.cpp

components/eamxx/src/share/io/scorpio_output.cpp

jgfouca

Approving based on the PR description. I ain't looking through 3000 lines, lol

bartgol · 2025-11-04T17:46:18Z

The diffs are expected, as the new impl of perturb is non-BFB w.r.t. to the previous impl, and we cannot make it BFB. The reason is that the previous impl used the same Engine obj for subsequent calls to randomize, while the new impl of randomize re-creates the engine obj at every call, requiring only the seed parameter as input. This was done to allow hiding away the impl (the engine input was a template arg, forcing the fcn impl to be in the header).

@tcclevenger You introduced the IC perturbation feature; are you ok with this specific mod?

The constructor of ScalarWrapper already takes care of type conversion

bartgol · 2025-11-10T15:48:30Z

Failures are expected, waiting for final review by @tcclevenger

This design pattern uses a type-agnostic wrapper for scalars, so that we don't need to expose the templated method implementation to the customers. [non-BFB] For runs using the IC perturb feature

bartgol added 7 commits October 31, 2025 14:35

EAMxx: make data type utils device friendly

8b31d10

EAMxx: small struct to store a scalar in an type-agnostic way

87bb7d3

Useful to pass around scalars without having to template every function on the passed scalar

EAMxx: add convenience shortcut for Field immediate allocation

935b1cb

EAMxx: start splitting field utils into separate files

b78dea4

Start from utils that are not templated

EAMxx: extract compute_mask util into its own cpp and unit test

f7795e4

EAMxx: extract field reduction utils into their own cpp unit test

242c42c

Use ScalarWrapper to hide implementation

EAMxx: extract field contractions utils into their own cpp and unit test

943e710

bartgol requested review from mahf708 and tcclevenger November 1, 2025 06:35

bartgol self-assigned this Nov 1, 2025

bartgol added BFB PR leaves answers BFB EAMxx C++ based E3SM atmosphere model (aka SCREAM) labels Nov 1, 2025

mahf708 requested review from Copilot and removed request for mahf708 and tcclevenger November 1, 2025 11:42

Copilot AI reviewed Nov 1, 2025

View reviewed changes

mahf708 approved these changes Nov 3, 2025

View reviewed changes

bartgol requested review from jgfouca and tcclevenger November 3, 2025 20:36

bartgol added 3 commits November 3, 2025 14:37

EAMxx: make randomization field util non-templated

88a8990

Use ScalarWrapper to hide the template implementation. The randomize util will ALWAYS use a uniform distribution. Use ranodmize_normal for a normal distribution

EAMxx: fix missing include

480c469

EAMxx: simplify API of perturb util, removing template arg

b08d7ec

bartgol force-pushed the bartgol/eamxx/field-utils branch from 32b8642 to b08d7ec Compare November 3, 2025 21:38

mahf708 requested a review from Copilot November 3, 2025 22:06

Copilot AI reviewed Nov 3, 2025

View reviewed changes

components/eamxx/src/share/field/utils/views_are_equal.cpp Show resolved Hide resolved

components/eamxx/src/share/io/scorpio_output.cpp Show resolved Hide resolved

components/eamxx/src/share/io/scorpio_output.cpp Show resolved Hide resolved

jgfouca approved these changes Nov 3, 2025

View reviewed changes

tcclevenger approved these changes Nov 4, 2025

View reviewed changes

bartgol removed the BFB PR leaves answers BFB label Nov 4, 2025

bartgol added the non-BFB PR makes roundoff changes to answers. label Nov 4, 2025

bartgol added 3 commits November 4, 2025 22:55

EAMxx: fix bug in IOP forcing process

372467c

EAMxx: remove pointless templated overloads of field utils

f5cfd03

The constructor of ScalarWrapper already takes care of type conversion

EAMxx: remove outdated and unused files

890b0b3

bartgol requested a review from tcclevenger November 8, 2025 03:18

tcclevenger approved these changes Nov 11, 2025

View reviewed changes

bartgol merged commit 021660a into master Nov 11, 2025
13 of 25 checks passed

bartgol deleted the bartgol/eamxx/field-utils branch November 11, 2025 18:22

		EKAT_KERNEL_ERROR_MSG ("Error! Unsupported data type.\n");
		return DataType::Invalid;

EAMxx: hide field utils impl using a type-agnostic scalar wrapper #7857

EAMxx: hide field utils impl using a type-agnostic scalar wrapper #7857

Uh oh!

Conversation

bartgol commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

mahf708 left a comment

Choose a reason for hiding this comment

Uh oh!

bartgol commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahf708 commented Nov 3, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgfouca left a comment

Choose a reason for hiding this comment

Uh oh!

bartgol commented Nov 4, 2025

Uh oh!

bartgol commented Nov 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bartgol commented Nov 1, 2025 •

edited

Loading

bartgol commented Nov 3, 2025 •

edited

Loading