dot product along a rank-1 field #3084
Conversation
mahf708
changed the title
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
SCREAM_PullRequest_Autotester_Weaver # 6266 FAILED (click to see last 100 lines of console output) |
| template <typename ST> | ||
| Field dot_along_rank1_dim(const int &pd, const Field &f1, const Field &f2, | ||
| const ekat::Comm *co = nullptr) { | ||
| return do_dot_along_rank1_dim<ST>(pd, f1, f2, co); |
There was a problem hiding this comment.
I would say add the implementation to the impl namespace like is done with the other utils, and just give the same name as this function.
There was a problem hiding this comment.
I considered namespacing impl, impl_dot and nothing, and chose nothing for no particular reason, but I will revisit. Let's see what Luca suggests. If he doesn't have a preference, I will go with impl as you suggested
There was a problem hiding this comment.
I also vote for using the impl namespace and keep the same signature.
| << l2.dim(pd) | ||
| << " \n" | ||
| "along the provided dimension pd " | ||
| << pd << " .\n"); |
There was a problem hiding this comment.
Maybe add the checks above to the dot_along_rank1_dim() function like with the other utils.
| #include "share/field/field.hpp" | ||
|
|
||
| namespace scream { | ||
|
|
There was a problem hiding this comment.
Here's where I would add namespace impl {. See field_utils_impl.hpp.
| const auto &u2 = f2.get_header().get_identifier().get_units(); | ||
| const auto &g2 = f2.get_header().get_identifier().get_grid_name(); | ||
|
|
||
| EKAT_REQUIRE_MSG(pd <= 5 && pd >= 0, |
There was a problem hiding this comment.
I wonder if we should make this super generic. The impl below is quite long mostly b/c of handling different values of pd. What if we only supported pd=0 (for contraction along horiz dim) and pd=rank-1 (for contraction along vertical dim)? We could even make two separate methods, to make the code more self-explanatory:
Field horiz_contracttion (const Field& field, const Field& weight);
Field vert_contracttion (const Field& field, const Field& weight);or something like that. We'd of course check that the input fields have the correct tag (COL for horiz and LEV/ILEV for vert), that it is at the expected, and that the extent match. We could also make it so that the weight field has JUST the horiz (resp. vert) dimension, so that it is indeed a weight. Maybe we should make weight optional too, so
Field horiz_contraction (const Field& field, const Field* weight = nullptr);I think this would cover most of our potential use cases.
There was a problem hiding this comment.
two comments:
- are we always leading with COL and ending with LEV in EAMxx? I didn't assume that in the impl
- I see this potentially useful for one additional CMP dim (say swband)
If the answer to the first is YES, then I can simplify this impl into the following:
- only allow pg, and only allow up to 3 dims a permutation of COL, LEV, CMP
- contract_field_along_col (essentially as above, but pd = 0)
- contract_field_along_lev (essentially as above, but pd = rank-1 = 2)
- contract_field_along_cmp (essentially as above, but pd = 1)
But this is essentially just the same as saying, cut the condition on pd from [1,5] to [1,3], which is fine with me. We won't gain much simplicity, right?
Thoughts?
There was a problem hiding this comment.
We baked the assumptions that "COL comes first and LEV/ILEV come last" in a lot of places, so I think it's safe to assume that will remain the case.
There was a problem hiding this comment.
contract_field_along_lev (essentially as above, but pd = rank-1 = 2)
Well, LEV could be at pd=0 (for a (LEV) field), 1 (for a (COL,LEV) field) ,or 2 (for a (COL,CMP,LEV) field), so we cannot assume it's 2. But we can assume it's rank-1. OTOH, for CMP we can prob assume it is at position 1, for now.
There was a problem hiding this comment.
For reduction along CMP, assuming that the extent is "small", I would consider doing something like
Field out = f.get_component(0).clone(); // get output layout
out.deep_copy(0);
for (int icmp=0; icmp<num_cmp; ++icmp) {
out.update(f.get_component(icmp),1,1);
}Requires num_cmp loops, but they are ALL ||for, no ||reduce.
Edit: obvious mods to add also a weight field.
There was a problem hiding this comment.
So, gimme your final advice: You don't like the complex generic impl along an arbitrary axis and I should try to produce a simple one that does narrow usecases in isolation?
There was a problem hiding this comment.
I think so. I doubt we will ever need to reduce along a generic dimension. Our only two/three cases are COL, LEV/ILEV, and CMP. The latter is prob easier to impl via for loop and calls to update; the COL case can assume it's the 1st dim; and the LEV/ILEV case can assume it's the last dim. And we can assume we contract a single field, possibly weighted by a 1d field (of length equal to the extent along which we are contracting).
There was a problem hiding this comment.
As you said, these use cases will cover our most likely usage: horizontal sum/avg, vertical sum/avg, and reduction across bands (or other CMP extents).
bartgol
force-pushed
the
master
branch
2 times, most recently
from
7114e81 to
579850c
Compare
|
to be revisited in a few weeks. |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 Enforce checks passingThis rule is failing.Make sure that checks are not failing on the PR, and reviewers approved
|
came up in #3068