Add UnorderedMapInsertOps for coo2crs #5877

e10harvey · 2023-02-13T19:40:50Z

To implement the sparse conversion kernel, coo2crs, I would like to have duplicate key insertions accumulate values in a Kokkos::UnorderedMap. This PR adds UnorderedMapInsertOps::AtomicAdd and UnorderedMapInsertOps::NoOp (the current insertion behavior for duplicate keys).

Related to kokkos/kokkos-kernels#1164.

Going forward, I would also like to extend Kokkos::UnorderedMap to support shared memory and add UnorderedMapInsertOps::Overwrite.

containers/src/Kokkos_UnorderedMap.hpp

containers/unit_tests/TestUnorderedMap.hpp

containers/src/Kokkos_UnorderedMap.hpp

masterleinad · 2023-02-14T18:45:13Z

Apparently, this fails on the OpenMP backend and you will also need to fix the indentation. 🙂

containers/src/Kokkos_UnorderedMap.hpp

masterleinad · 2023-02-15T18:43:37Z

Looks like there is still a segfault (at least for OpenMP).

containers/unit_tests/TestUnorderedMap.hpp

containers/src/Kokkos_UnorderedMap.hpp

crtrott

The proposed approach is a breaking change: why can't we just have an insert function overload which takes an op, or add a insert_accumulate function?

containers/src/Kokkos_UnorderedMap.hpp

crtrott · 2023-02-18T22:34:10Z

containers/src/Kokkos_UnorderedMap.hpp

+              Kokkos::View<std::remove_const_t<std::conditional_t<
+                               std::is_void_v<Value>, int, Value>> *,
+                           Device>,
+              uint32_t>::NoOp>


Adding a template parameter is a breaking change we can't do that really. We could discuss adding this only if deprecated code off, but we can't simply add it. Alternatively, why can't we have an overload of insert which takes an insert op? Is there a reason this has to be a global template for the class?

This seems like the lightest-weight way to add this operation as well as following the implementation style of UnorderedMap. What about keeping the current approach with a single constructor overload, rather than a new insert method, the constructor overload could include the InsertOp type and constructor parameter?

Thinking about this more now, I see no reason for making the InsertOp a global template for the class other than following the existing implementation style of UnorderedMap (e.g. EqualTo, etc.). I will try refactoring with a new insert method instead. Can you elaborate on how the InsertOp template parameter is a breaking change?

After trying to remove the InsertOp as a global template parameter, I found that deep copy no longer works. I will look into using insert method overloads rather than a class member.

containers/src/Kokkos_UnorderedMap.hpp

crtrott · 2023-02-18T22:38:33Z

containers/src/Kokkos_UnorderedMap.hpp

@@ -532,6 +593,7 @@ class UnorderedMap {
        }

        result.set_existing(curr, free_existing);
+        if (!is_set) m_insert_op.op(m_values, curr, &v);


yeah this is the only place where this is used. I think we can just have an overload of insert which takes the insert op. Or maybe even have a different name: insert_accumulate or so. Do you see any issue with that approach?

The only issue I see would be a lot of duplicate code.

e10harvey · 2023-02-21T23:42:15Z

This is ready for another round of reviews. CI tests for 87c3691 will fail until #5899 merges.

crtrott · 2023-03-15T16:17:55Z

Needs a rebase. Question at everyone but especially @masterleinad @nmm0 @nliber @dalg24: Do we like the way the insert ops are grouped here mechanically as nested classes in a separate class?

There are alternatives:

nested namespace
make them nested classes in UnorderedMap itself
named ops (this is what ScatterView does where the ops are called ScatterAdd etc. in the Kokkos::Experimental namespace)

At least we should check for where we do other stuff like that and what approach we take so we can end up with a consistent way of doing this.

crtrott · 2023-03-15T16:23:30Z

Oh just to confirm: the overload approach of insert with a default insert mechanism is good now.

nmm0 · 2023-03-15T16:37:04Z

Needs a rebase. Question at everyone but especially @masterleinad @nmm0 @nliber @dalg24: Do we like the way the insert ops are grouped here mechanically as nested classes in a separate class?

There are alternatives:
* nested namespace

* make them nested classes in UnorderedMap itself

* named ops (this is what ScatterView does where the ops are called ScatterAdd etc. in the Kokkos::Experimental namespace)
At least we should check for where we do other stuff like that and what approach we take so we can end up with a consistent way of doing this.

I guess one question is whether we want this to be a point of customization. I'm not saying we should but if we want additional ops to be added we can't use it in a nested class.

nmm0

Could you add a test for void value'd maps (i.e. sets)? What is the expected behavior of the ops when value is void?

containers/src/Kokkos_UnorderedMap.hpp

e10harvey · 2023-03-15T18:00:54Z

Could you add a test for void value'd maps (i.e. sets)? What is the expected behavior of the ops when value is void?

Hi @nmm0. Thanks for your review. The op is not executed when the map is void value'd due to if(!is_set). This is already tested and handled the same way it was before this PR was opened. The specific line of code introduced for getting void value'd maps with these insert ops to still build is: https://github.com/kokkos/kokkos/pull/5877/files#diff-2c8acfc1ac5cbbb4cc3da14e466f42c85e415b540c815bf2630c42ec2ac09d59R576.

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

Fix OpenMP test failures Fix CI build error Increase size of expected_values

nmm0 · 2023-03-15T18:13:06Z

Could you add a test for void value'd maps (i.e. sets)? What is the expected behavior of the ops when value is void?

Hi @nmm0. Thanks for your review. The op is not executed when the map is void value'd due to if(!is_set). This is already tested and handled the same way it was before this PR was opened. The specific line of code introduced for getting void value'd maps with these insert ops to still build is: https://github.com/kokkos/kokkos/pull/5877/files#diff-2c8acfc1ac5cbbb4cc3da14e466f42c85e415b540c815bf2630c42ec2ac09d59R576.

Thanks for the clarification!

Let's then do:

if constexpr (!is_set) {
  arg_insert_op.op(m_values, curr, v);
}

Then we don't have to worry about it compiling with a set. Also, in that case, I think we should probably not allow the op to be even specified if inserting into a set.

This is something we probably need to deal with now, because if a user specifies an InsertOp for a set and we clean up the interface later then it would be a breaking change.

I'd like to get other people's opinion on it but one suggestion would be to just do a static_assert, which would be a quick fix rather than making an overload:

if constexpr (is_set) {
  static_assert(std::is_same_v<InsertOpType, default_op_type>);
}

Ensure m_insert_op gets copied too

e10harvey · 2023-03-16T14:19:21Z

The Jenkins SYCL-OneApi failure is unrelated to these changes:

43: [ RUN      ] std_algorithms_sorting_ops_test.is_sorted_until
43: is_sorted_until: dynamic_view, all overloads 
43: /var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsIsSortedUntil.cpp:150: Failure
43: Expected equality of these values:
43:   r3
43:     Which is: 32-byte object <70-0B C3-02 00-00 00-00 80-00 A0-93 9C-7F 00-00 89-8C 01-00 00-00 00-00 F9-02 00-00 00-00 00-00>
43:   gold
43:     Which is: 32-byte object <70-0B C3-02 00-00 00-00 80-00 A0-93 9C-7F 00-00 89-8C 01-00 00-00 00-00 9C-00 00-00 00-00 00-00>
43: /var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsIsSortedUntil.cpp:151: Failure
43: Expected equality of these values:
43:   r4
43:     Which is: 32-byte object <70-0B C3-02 00-00 00-00 80-00 A0-93 9C-7F 00-00 89-8C 01-00 00-00 00-00 01-03 00-00 00-00 00-00>
43:   gold
43:     Which is: 32-byte object <70-0B C3-02 00-00 00-00 80-00 A0-93 9C-7F 00-00 89-8C 01-00 00-00 00-00 9C-00 00-00 00-00 00-00>
43: /var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsIsSortedUntil.cpp:165: Failure
43: Expected equality of these values:
43:   r3
43:     Which is: 32-byte object <70-0B C3-02 00-00 00-00 80-00 A0-93 9C-7F 00-00 89-8C 01-00 00-00 00-00 F9-02 00-00 00-00 00-00>
43:   gold
43:     Which is: 32-byte object <70-0B C3-02 00-00 00-00 80-00 A0-93 9C-7F 00-00 89-8C 01-00 00-00 00-00 9C-00 00-00 00-00 00-00>
43: /var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsIsSortedUntil.cpp:166: Failure
43: Expected equality of these values:
43:   r4
43:     Which is: 32-byte object <70-0B C3-02 00-00 00-00 80-00 A0-93 9C-7F 00-00 89-8C 01-00 00-00 00-00 01-03 00-00 00-00 00-00>
43:   gold
43:     Which is: 32-byte object <70-0B C3-02 00-00 00-00 80-00 A0-93 9C-7F 00-00 89-8C 01-00 00-00 00-00 9C-00 00-00 00-00 00-00>
43: is_sorted_until: stride2_view, all overloads 
43: is_sorted_until: stride3_view, all overloads 
43: [  FAILED  ] std_algorithms_sorting_ops_test.is_sorted_until (72 ms)

Full console: https://cloud.cees.ornl.gov/jenkins-ci/blue/rest/organizations/jenkins/pipelines/Kokkos/runs/12424/nodes/49/steps/252/log/?start=0.

e10harvey · 2023-03-20T13:24:04Z

The Jenkins SYCL-OneApi failed again:

43: [ RUN      ] std_algorithms_sorting_ops_test.is_sorted_until
43: is_sorted_until: dynamic_view, all overloads 
43: /var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsIsSortedUntil.cpp:148: Failure
43: Expected equality of these values:
43:   r1
43:     Which is: 32-byte object <40-15 04-02 00-00 00-00 80-00 A0-B3 AD-7F 00-00 89-8C 01-00 00-00 00-00 89-8C 01-00 00-00 00-00>
43:   gold
43:     Which is: 32-byte object <40-15 04-02 00-00 00-00 80-00 A0-B3 AD-7F 00-00 89-8C 01-00 00-00 00-00 9C-00 00-00 00-00 00-00>
43: /var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsIsSortedUntil.cpp:163: Failure
43: Expected equality of these values:
43:   r1
43:     Which is: 32-byte object <40-15 04-02 00-00 00-00 80-00 A0-B3 AD-7F 00-00 89-8C 01-00 00-00 00-00 89-8C 01-00 00-00 00-00>
43:   gold
43:     Which is: 32-byte object <40-15 04-02 00-00 00-00 80-00 A0-B3 AD-7F 00-00 89-8C 01-00 00-00 00-00 9C-00 00-00 00-00 00-00>
43: is_sorted_until: stride2_view, all overloads 
43: is_sorted_until: stride3_view, all overloads 
43: [  FAILED  ] std_algorithms_sorting_ops_test.is_sorted_until (143 ms)

Can anyone confirm that this failure is unrelated to these changes?

masterleinad · 2023-03-20T13:35:01Z

I would think that the failure is unrelated; the test is not using Kokkos::UnorderedMap.

containers/src/Kokkos_UnorderedMap.hpp

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

e10harvey · 2023-03-20T21:06:23Z

I think this is ready to merge. The OpenMP Target CI failure in Jenkins appears to be unrelated to these change.

masterleinad

Looks OK to me.

nmm0 · 2023-03-22T18:39:03Z

Needs a rebase. Question at everyone but especially @masterleinad @nmm0 @nliber @dalg24: Do we like the way the insert ops are grouped here mechanically as nested classes in a separate class?

There are alternatives:
* nested namespace

* make them nested classes in UnorderedMap itself

* named ops (this is what ScatterView does where the ops are called ScatterAdd etc. in the Kokkos::Experimental namespace)
At least we should check for where we do other stuff like that and what approach we take so we can end up with a consistent way of doing this.

@crtrott did you get satisfactory feedback on this?

nmm0

I think this is ready depending on what we decide for the convention for Ops

However it needs a changelog entry (4.1) and a corresponding PR for the docs

e10harvey · 2023-03-23T15:11:47Z

I think this is ready depending on what we decide for the convention for Ops

However it needs a changelog entry (4.1) and a corresponding PR for the docs

Thinking about Christian's question some more now and in particular how ScatterView handles insertion operators, we should be consistent if possible. @nmm0, @dalg24: Is ScatterView the only other container that supports "operations" in Kokkos core?

nmm0

Thanks for adding docs and changelog!

e10harvey · 2023-03-23T15:26:53Z

I think this is ready depending on what we decide for the convention for Ops
However it needs a changelog entry (4.1) and a corresponding PR for the docs

Thinking about Christian's question some more now and in particular how ScatterView handles insertion operators, we should be consistent if possible. @nmm0, @dalg24: Is ScatterView the only other container that supports "operations" in Kokkos core?

It looks like ScatterView has the concept of a ScatterValue type and specializes ScatterValue for the supported "operation" types like ScatterSum. Thinking about how to support extension of insertion operations in this way is not clear to me. Before this PR, the Kokkos::UnorderedMap only required operator= for the given value_type. With this PR, we only change what operation is used if the given key already exists. So, from my perspective, I think the current approach using arg_insert_op is cleaner than introducing the concept of a ValueType with a ValueOp to UnorderedMap since the body of insert and UnorderedMap are not exposed so generally in the form of a data-type with operator overloads like ScatterView is. In a later PR, we could revisit UnorderedMapInsertOpTypes if they are found to be difficult to use and extend.

dalg24 · 2023-03-28T19:39:01Z

Retest this please

PhilMiller · 2023-03-28T19:49:06Z

Retest this please

PhilMiller · 2023-03-28T19:49:21Z

Rerunning the tests, since it looks like Jenkins had a hiccup

e10harvey · 2023-03-29T18:06:06Z

Retest this please

e10harvey · 2023-03-30T12:52:33Z

GCC 8.4.0 timed out and the SYCL-OneApi test failed again:

43: is_sorted_until: dynamic_view, all overloads 
43: /var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsIsSortedUntil.cpp:149: Failure
43: Expected equality of these values:
43:   r2
43:     Which is: 32-byte object <10-54 8F-02 00-00 00-00 80-00 A0-D3 9A-7F 00-00 89-8C 01-00 00-00 00-00 E1-02 00-00 00-00 00-00>
43:   gold
43:     Which is: 32-byte object <10-54 8F-02 00-00 00-00 80-00 A0-D3 9A-7F 00-00 89-8C 01-00 00-00 00-00 9C-00 00-00 00-00 00-00>
43: /var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsIsSortedUntil.cpp:164: Failure
43: Expected equality of these values:
43:   r2
43:     Which is: 32-byte object <10-54 8F-02 00-00 00-00 80-00 A0-D3 9A-7F 00-00 89-8C 01-00 00-00 00-00 E1-02 00-00 00-00 00-00>
43:   gold
43:     Which is: 32-byte object <10-54 8F-02 00-00 00-00 80-00 A0-D3 9A-7F 00-00 89-8C 01-00 00-00 00-00 9C-00 00-00 00-00 00-00>
43: is_sorted_until: stride2_view, all overloads 
43: is_sorted_until: stride3_view, all overloads 
43: [  FAILED  ] std_algorithms_sorting_ops_test.is_sorted_until (74 ms)

e10harvey requested review from masterleinad, crtrott, srajama1 and lucbv February 13, 2023 19:40

e10harvey self-assigned this Feb 13, 2023

masterleinad reviewed Feb 13, 2023

View reviewed changes

masterleinad reviewed Feb 14, 2023

View reviewed changes

containers/src/Kokkos_UnorderedMap.hpp Outdated Show resolved Hide resolved

containers/src/Kokkos_UnorderedMap.hpp Outdated Show resolved Hide resolved

masterleinad reviewed Feb 15, 2023

View reviewed changes

containers/src/Kokkos_UnorderedMap.hpp Outdated Show resolved Hide resolved

e10harvey force-pushed the umap_insert_ops branch from 3eba2a7 to 10d9942 Compare February 15, 2023 17:54

e10harvey force-pushed the umap_insert_ops branch from bc33872 to 9447ad0 Compare February 15, 2023 18:56

e10harvey mentioned this pull request Feb 15, 2023

sparse: Add coo2crs, crs2coo and CooMatrix kokkos/kokkos-kernels#1686

Merged

masterleinad reviewed Feb 15, 2023

View reviewed changes

containers/unit_tests/TestUnorderedMap.hpp Outdated Show resolved Hide resolved

e10harvey force-pushed the umap_insert_ops branch from 9447ad0 to 3522dfe Compare February 16, 2023 15:26

dalg24 reviewed Feb 16, 2023

View reviewed changes

containers/src/Kokkos_UnorderedMap.hpp Outdated Show resolved Hide resolved

containers/src/Kokkos_UnorderedMap.hpp Outdated Show resolved Hide resolved

containers/src/Kokkos_UnorderedMap.hpp Outdated Show resolved Hide resolved

crtrott requested changes Feb 18, 2023

View reviewed changes

e10harvey force-pushed the umap_insert_ops branch from 3522dfe to 2177292 Compare February 21, 2023 21:23

e10harvey mentioned this pull request Mar 13, 2023

UnorderedMap: Work around mutable m_size corruption bug #5899

Closed

nmm0 requested changes Mar 15, 2023

View reviewed changes

containers/src/Kokkos_UnorderedMap.hpp Outdated Show resolved Hide resolved

e10harvey and others added 3 commits March 15, 2023 12:11

Add UnorderedMapInsertOps for coo2crs

24a59f2

Apply suggestions from code review

7e42970

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

Implement PR feedback

239b3c4

Fix OpenMP test failures Fix CI build error Increase size of expected_values

Create host mirrors for checking expected values

96fb86b

Ensure m_insert_op gets copied too

e10harvey force-pushed the umap_insert_ops branch from 87c3691 to c265bdb Compare March 15, 2023 19:34

e10harvey added 2 commits March 16, 2023 07:58

Silence intel error 869

5a40910

Reduce unit test runtime

9a64e77

e10harvey requested review from nmm0, crtrott, masterleinad and dalg24 March 20, 2023 13:24

masterleinad reviewed Mar 20, 2023

View reviewed changes

containers/src/Kokkos_UnorderedMap.hpp Outdated Show resolved Hide resolved

containers/src/Kokkos_UnorderedMap.hpp Outdated Show resolved Hide resolved

e10harvey and others added 2 commits March 20, 2023 07:49

Update containers/src/Kokkos_UnorderedMap.hpp

2534b95

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

Fix typo

cb6daef

e10harvey requested a review from masterleinad March 22, 2023 14:40

masterleinad approved these changes Mar 22, 2023

View reviewed changes

nmm0 requested changes Mar 22, 2023

View reviewed changes

nmm0 added this to the Release 4.1 milestone Mar 22, 2023

This was referenced Mar 23, 2023

CHANGELOG: 4.1.0 #5902

Closed

Update Unordered-Map.md kokkos/kokkos-core-wiki#350

Closed

nmm0 approved these changes Mar 23, 2023

View reviewed changes

dalg24 merged commit fdb089b into kokkos:develop Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UnorderedMapInsertOps for coo2crs #5877

Add UnorderedMapInsertOps for coo2crs #5877

e10harvey commented Feb 13, 2023

masterleinad commented Feb 14, 2023

masterleinad commented Feb 15, 2023

crtrott left a comment

crtrott Feb 18, 2023

e10harvey Feb 21, 2023

e10harvey Feb 21, 2023

e10harvey Feb 21, 2023

crtrott Feb 18, 2023

e10harvey Feb 21, 2023

e10harvey commented Feb 21, 2023

crtrott commented Mar 15, 2023

crtrott commented Mar 15, 2023

nmm0 commented Mar 15, 2023

nmm0 left a comment

e10harvey commented Mar 15, 2023

nmm0 commented Mar 15, 2023

e10harvey commented Mar 16, 2023

e10harvey commented Mar 20, 2023

masterleinad commented Mar 20, 2023

e10harvey commented Mar 20, 2023 •

edited

masterleinad left a comment

nmm0 commented Mar 22, 2023

nmm0 left a comment

e10harvey commented Mar 23, 2023

nmm0 left a comment

e10harvey commented Mar 23, 2023 •

edited

dalg24 commented Mar 28, 2023

PhilMiller commented Mar 28, 2023

PhilMiller commented Mar 28, 2023

e10harvey commented Mar 29, 2023

e10harvey commented Mar 30, 2023

Add UnorderedMapInsertOps for coo2crs #5877

Add UnorderedMapInsertOps for coo2crs #5877

Conversation

e10harvey commented Feb 13, 2023

masterleinad commented Feb 14, 2023

masterleinad commented Feb 15, 2023

crtrott left a comment

Choose a reason for hiding this comment

crtrott Feb 18, 2023

Choose a reason for hiding this comment

e10harvey Feb 21, 2023

Choose a reason for hiding this comment

e10harvey Feb 21, 2023

Choose a reason for hiding this comment

e10harvey Feb 21, 2023

Choose a reason for hiding this comment

crtrott Feb 18, 2023

Choose a reason for hiding this comment

e10harvey Feb 21, 2023

Choose a reason for hiding this comment

e10harvey commented Feb 21, 2023

crtrott commented Mar 15, 2023

crtrott commented Mar 15, 2023

nmm0 commented Mar 15, 2023

nmm0 left a comment

Choose a reason for hiding this comment

e10harvey commented Mar 15, 2023

nmm0 commented Mar 15, 2023

e10harvey commented Mar 16, 2023

e10harvey commented Mar 20, 2023

masterleinad commented Mar 20, 2023

e10harvey commented Mar 20, 2023 • edited

masterleinad left a comment

Choose a reason for hiding this comment

nmm0 commented Mar 22, 2023

nmm0 left a comment

Choose a reason for hiding this comment

e10harvey commented Mar 23, 2023

nmm0 left a comment

Choose a reason for hiding this comment

e10harvey commented Mar 23, 2023 • edited

dalg24 commented Mar 28, 2023

PhilMiller commented Mar 28, 2023

PhilMiller commented Mar 28, 2023

e10harvey commented Mar 29, 2023

e10harvey commented Mar 30, 2023

e10harvey commented Mar 20, 2023 •

edited

e10harvey commented Mar 23, 2023 •

edited