atlas
atlas copied to clipboard
ecmwf
Added function to create `std::variant` for multiple array views.
During PR #218, I found myself writing two nested switch statements in order to create an ArrayView from an Array. I've found myself writing this same pattern multiple times throughout Atlas, and indeed other external applications which use atlas.
This PR adds provides an alternative to the nested switch statement. The method util::make_array_view_variant<Values<types...>, Ranks<ints...>>(Array& array) creates an std::variant of ArrayView types, and then assigns the view that matches array (so long as the value-type and rank can be found within the Values and Ranks arguments respectively. The ArrayView can then be accessed rather succinctly using std::visit.
Hi @odlomax I think this is a great idea.
I was wondering if this should not become part of the "array" namespace. If you want we could discuss offline.
It should also cater for device views though.
array::make_host_view_variant and array::make_device_view_variant
All possibly supported Values and Ranks could be immediately part of the variant type, removing even more boiler-plate?
Hi @odlomax I think this is a great idea. I was wondering if this should not become part of the "array" namespace. If you want we could discuss offline. It should also cater for device views though.
array::make_host_view_variantandarray::make_device_view_variantAll possibly supported Values and Ranks could be immediately part of the variant type, removing even more boiler-plate?
Hi @wdeconinck,
Thanks for the feedback! I'm on leave at the moment, but this is oddly recreational between baby's naps. I'll arrange a catch up when I'm back after next week. 🙂
Hi @wdeconinck. I believe I've incorporated all of your suggestions. It's ready for a look when you have the time. Cheers!
Codecov Report
Attention: Patch coverage is 92.96875% with 9 lines in your changes missing coverage. Please review.
Project coverage is 80.02%. Comparing base (
afd3b5f) to head (73d990f). Report is 14 commits behind head on develop.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/tests/array/test_array_view_variant.cc | 92.70% | 7 Missing :warning: |
| src/atlas/array/ArrayViewVariant.cc | 93.75% | 2 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## develop #220 +/- ##
===========================================
+ Coverage 79.95% 80.02% +0.06%
===========================================
Files 792 794 +2
Lines 62309 62491 +182
===========================================
+ Hits 49822 50011 +189
+ Misses 12487 12480 -7
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@wdeconinck Apologies for the last minute push.
There were relatively important EXPECT statements that needed to be added to lines 139 and 164 of the test. I also removed some extraneous whitespace at the end of an otherwise untouched file.
Hi @wdeconinck,
When attempting to using this in anger (see PR #226), I found that it was worth refactoring the make_view_variant methods to produce either const or non-const versions of the variant depending on the constness of the array. Shall I merge the refactor into this PR, or leave it in #226?
Hi @wdeconinck @sbrdar,
I've refactored the introspection helpers as we discussed on Slack. I've also added an additional helper to distinguish between value_type and non_const_value_type. Seemed like a sensible idea.
Cheers!
@wdeconinck the latest push has your suggested signature for the introspection helpers. Thanks for the suggestion, it's a lot tidier.
Basic GHA is passing now!
https://github.com/JCSDA-internal/atlas/actions/runs/11157940876
MacOS tests! Nooo!
https://github.com/ecmwf/atlas/actions/runs/11157941275/job/31068813417?pr=220 should not be the problem of this PR, but about the CI infrastructure. I passed on this problem.