@PhilMiller: I think I see how to rewrite the UnorderedMap deep copy specialization. Just to confirm we're on the same page: for the handful of UnorderedMap internal members that are of type Kokkos::View, the UnorderedMap deep copy specialization must allocate new views before calling deep copy on those internal members, correct?

No it is kinda weird that it does. I can somewhat understand why since the unordered map grows somewhat dynamically internally, but the idea is to make it behave more like normal views.

In Kokkos_CopyViews.hpp I see that there are multiple specializations of deep_copy depending on the traits that indicate where the dst and src views reside, or if you want to copy a single value to/from the host, or if you are copying between views with the same contiguous layout, etc.

For UnorderedMap, we only have a single deep_copy specialization. Additionally, the View deep_copy overloads are not a member of the View data type, like they are in UnorderedMap. And, the UnorderedMap deep_copy member allocates views internally (from the destination map object) and then calls Impl::DeepCopy on the newly allocated internal views without checking whether the dst and src views overlap. All of this being said, I think the main issue as identified by Phil and Christian in this bug report is that the UnorderedMap deep_copy member creates a local insertable_map_type tmp. Then, after copying all the src members to tmp it assigns this tmp object to the dst object we are executing within. With this implementation, I am unsure what the expected behavior is. Note that the current template parameters of UnorderedMap do not support specifying the layout of the internal views but do allow specifying the Device.

I propose that we address this bug as follows:

1. Create four UnorderedMap copy constructors in Kokkos_UnorderedMap.hpp for host->host (done via current copy constructor?), host->device, device->host, and device->device (done via current copy constructor?). These copy constructors would check that the internal view lengths, layouts, etc match. If these checks pass, then the copy constructor would rely on Kokkos::deep_copy from Kokkos_CopyViews.hpp to copy all internal views from src to dst.
2. Create a single catch-all templated deep_copy overload (within the Kokkos namespace, not the UnorderedMap class) in Kokkos_UnorderedMap.hpp. This deep_copy overload would check public UnorderedMap template arguments, types, and members to ensure that the copy can be done and then call the copy constructor. I suggest this rather than four deep_copy specializations since the deep_copy function cannot access internal members of the UnorderedMap class unless we make deep_copy a class member. Also, I think using copy constructors to do the actual copying is more inline with how the standard library C++ unordered_map class works.

Language-wise, the signature for a copy constructor is C(C const&) (or the much rarer C(C&), C(C volatile&) and C(C const volatile&). Specifically, it can't be a template. If there is an exact match for both a copy constructor and a templated constructor, the copy constructor is picked. If there isn't an exact match for the copy constructor but there is one for the templated constructor, then the templated constructor is picked.

So instead of copy constructors, I assume you mean four templated constructors to do the copying, sometimes calling the actual copy constructor (when the source and destination template parameters match, assuming the signature takes it by const reference).

I think we are just clearer with named functions. Mixing templated and non-templated functions, especially constructors, is error-prone.

From today's meeting, one possibility considered was to introduce a new name for the current functionality, reflecting that it's more complicated than other deep_copy overloads, and deprecate the current deep_copy(UnorderedMap).

The tension this issue faces is the incongruity between a consistency that users expect, of "I modified a data structure in one memory space, and I need the modified contents in another, so I'll deep_copy from one to the other", and the implementation consistency of "deep_copy strictly moves bytes around, and does not allocate or otherwise change any meta-data".

The ultimate conclusion was that more consideration is necessary to decide how to move forward, but the current functionality definitely needs to be available.

@ajpowelsnl, @PhilMiller, @nliber, @crtrott: Thanks for bringing this discussion up again and noting it here.

The ultimate conclusion was that more consideration is necessary to decide how to move forward, but the current functionality definitely needs to be available.

Does this mean that it was decided to not deprecate the behavior of deep_copy allocating a tmp internal UnorderedMap, copying src to tmp, and then assigning src to dst?

Having thought about this more now I vote for:

1. Deprecating the above behavior such that the caller must allocate both src and dst with the same capacities, Hasher types, and EqualTo types
2. Create four (h->h, h->d, d->h, d->d) deep_copy overloads that are friends of Kokkos::UnorderedMap

@e10harvey - your approach sounds good. In particular, please focus on these steps:

TODO:

• Replace existing functionality
• create_mirror_view & copy (note, not deep_copy)?
• Documentation for (Containers) UnorderedMap needs updating and extending

Hi @e10harvey - I added this issue to the Kokkos Planning Board. Can you update the issue, and perhaps guesstimate and ETA ?

@tcclevenger -- any updates on this fix / improvement?

@ajpowelsnl PR #6812 open but needs more reviews.