Add get_subview() Implementations for Multi-Sliced Subfields
#333
Conversation
|
Thanks for opening this pull request! Please check out our contributing guidelines. |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
jgfouca
added
the
AT: AUTOMERGE
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
mjs271
force-pushed
the
mjs/eamxx_multi-slice_subfield
branch
from
b8eff1d to
50d0869
Compare
|
Added tests for With this, I will also convert E3SM-Project/scream#2785 from draft to a full PR. |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
| const Kokkos::pair<int, int> &kp0, | ||
| const int idim) { | ||
| assert(v.data() != nullptr); | ||
| assert(idim == 0); |
There was a problem hiding this comment.
I would remove idim argument for this case I think, since it is always 0. But ok if you or others disagree.
There was a problem hiding this comment.
I can't say I have strong opinions either way, other than a potential argument for consistent function signatures. Though I'm not sure that is much of a big deal
Any thought from others?
There was a problem hiding this comment.
I would remove it. One rule I like when coding is "do not expose parameters/args unless the user is allowed to change/set them". If this parameter cannot be set by the user, then it should not be part of the function signature.
There was a problem hiding this comment.
So... there's probably something I'm doing wrong, but getting rid of the idim argument caused issues with resolving the choice of overloaded function. I'm guessing it's due to handling it within field_impl.hpp::get_multi_slice_view() using an if block, i.e.,
if (idim == 0 && v_fullsize.rank == 1) {
return DstView(ekat::subview_m1(v_fullsize,
Kokkos::make_pair<int, int>(k_beg, k_end)));
} else {
return DstView(ekat::subview(
v_fullsize, Kokkos::make_pair<int, int>(k_beg, k_end), idim));
}
In light of that, I'm happy to accept a suggestion for a fix that eliminates the extraneous argument, but in the mean time added a default value for the idim argument, and this makes the overloading straightforward again.
There was a problem hiding this comment.
Yeah, I see now that the way you had it allowed SCREAM side to not have any if-else.
What is the ekat::subview_m1(), is that a typo? If you have an if-else on the SCREAM side, there should be no reason to have the idim input. What was the error you were getting?
If we keep the idim input, I think there needs to still be an assert that the user only uses idim=0, since they could use it thinking it does something else.
There was a problem hiding this comment.
Ah, it is a typo... kind of. That was my attempt to differentiate the two choices by naming them differently. However, my interpretation is that the if/then results in a compile-time ambiguity about which function will be chosen by different sized views, and so doesn't build.
Also, if I'm recalling correctly, when it was just named ekat::subview(const View<DT*> v, const kk::make_pair<int, int>(a, b)), the compiler was considering the single-slice 1D function, ekat::subview(const View<DT*> v, const int idx), as a candidate, which some stranger on the internet seemed to be saying is because the compiler can't decide whether to prioritize choosing a 2-argument overloaded function based on type vs. const-ness 🤷♂️
All that being said... I will add that assert() back in 🙂
|
Quick comment below I review: there are some commits from me in the PR?. I suspect you were working off a branch of mine that got merged since then. You may try to tenant on master if issues arise. |
| // the cast silences a warning, but may be unnecessary | ||
| assert(kp0.first >= 0 && kp0.first < kp0.second | ||
| && kp0.second <= v.extent_int(idim)); | ||
| if (idim == 0) { |
There was a problem hiding this comment.
If you don't like these conditionals, we could bake the index in the function name. Someone like subview_0 for subviewing along the host index vs subview_1 for sunburn l subviewing asking the second. I think in most (all?) use cases we will know which dimension we're subviewing anyways. @tcclevenger what do you think?
There was a problem hiding this comment.
My only issue is this would end up creating a lot of function, right? subview_1() would need 6 versions, subview_2() would have 5, etc. And then in the SCREAM-side PR we would end up having a long switch(idim) case 1:, case 2:, ... chain.
There was a problem hiding this comment.
Unless there's an advantage to changing, I suppose I'm in favor of leaving it as-is, but could certainly be convinced to go with the opposite case
There was a problem hiding this comment.
Unless @bartgol has push back I'd say keep this how it is, and then later we may rethink all the subview routines.
| @@ -544,4 +544,230 @@ TEST_CASE("subviews") { | |||
| } | |||
| } | |||
|
|
|||
| TEST_CASE("multi-slice subviews") { | |||
There was a problem hiding this comment.
You should add some tests that catch errors. Since we used assert instead of exceptions, we can't use REQUIRE_THROWS.
Perhaps we could write a separate test with the WILL_FAIL option, and run one bad op in there. We can't test more since the assert crashes at the first error.. but maybe we can put N bad calls and randomly choose one to execute. Over time, we'd have some sort of coverage. Of course, this test can only be added in debug mode, since asserts are not executed in release.
@jgfouca @tcclevenger do you have other ideas on how to test that asserts do indeed work?
There was a problem hiding this comment.
@bartgol , I recommend using the EKAT assert macros in ekat_assert.hpp.
EKAT_ASSERT(false); // throws a logic_error
That would allow us to use REQUIRE_THROWS. This won't work for EKAT_KERNEL_ASSERT since that's just a Kokkos::abort.
mjs271
force-pushed
the
mjs/eamxx_multi-slice_subfield
branch
from
3011c75 to
856f699
Compare
|
Congrats on merging your first pull request! We hope this is only the first of many to come. |
Motivation
This PR is associated with E3SM-Project/scream#2785 and provides
get_subview_<N>()implementations for getting a multi-slicedsubfield's underlying view. I've created a separate overload function for each view rank ofN = 1, ... 6and employ if statements within to differentiate between which dimension is being sliced. Using the logic blocks isn't my favorite solution from an efficiency standpoint, so I'd be happy to hear any suggestions about a better approach.Testing
The added functions are tested within EAMxx's tests for
Field-related functionality. As of submitting this PR, some more tests are required, but I wanted to prioritize getting this out with the draft-submission for the EAMxx PR.