Generalized domains #698

tpadioleau · 2024-12-05T17:57:36Z

PoC for #438

Steps:

remove internal_mdspan
implement Chunk and ChunkSpan with SupportType being a customization point
implement a StridedDiscreteDomain
implement a StorageDiscreteDomain
update algorithms

Not clear if we should define an equivalent of the slice operator taking a DiscreteDomain

cc @science-enthusiast

include/ddc/chunk_common.hpp

include/ddc/discrete_domain.hpp

tpadioleau · 2025-01-16T17:51:28Z

The feature is almost ready but the integration could be further improved:

duplicated code due to specialization,
the name of some variables do not reflect well what they represent,
the new discrete domains StridedDiscreteDomain and StorageDiscreteDomain do not yet support slicing with DiscreteElement whereas it should not be difficult to extend

A slight overhead might be observed in algorithms in order to support the generalized discrete domains.

It is out of scope of the current PR but a future work should allow to slice a ChunkSpan<T, DiscreteDomain<...>> with a strided range of DiscreteVector to end up with a ChunkSpan<T, StridedDiscreteDomain<...>>:

ChunkSpan<T, DiscreteDomain<DDim1, DDim2>> chk_span1;
ChunkSpan<T, StridedDiscreteDomain<DDim1>> chk_span2 = chk_span1[strided_slice(DiscreteVector<DDim1>(2), DiscreteVector<DDim1>(10), DiscreteVector<DDim1>(2)][DiscreteVector<DDim2>(2)];

similar to https://en.cppreference.com/w/cpp/container/mdspan/strided_slice

…m_reduce

science-enthusiast · 2025-01-27T12:41:46Z

include/ddc/storage_discrete_domain.hpp

+
+template <class InputIt1, class InputIt2>
+KOKKOS_FUNCTION bool equal(InputIt1 first1, InputIt1 last1, InputIt2 first2)
+{


Could a is_pointer_v based concept help with better usage of this free function?

Discussed in person, I don't think so because it would require to assume an implementation detail on Kokkos iterators.

science-enthusiast · 2025-01-27T13:00:29Z

include/ddc/storage_discrete_domain.hpp

+            count -= step + 1;
+        } else {
+            count = step;
+        }


Could count be updated as count = last - first just after this if-then-else construct? first could be updated as it is being updated right now and last could be updated inside the else block as last = it. In this way, count is updated in the same manner it was initialized.

I don't, to be honest I copy-pasted the version found on cppref

science-enthusiast · 2025-01-27T13:15:01Z

include/ddc/storage_discrete_domain.hpp

+KOKKOS_FUNCTION ForwardIt lower_bound(ForwardIt first, ForwardIt last, const T& value, Compare comp)
+{
+    ForwardIt it;
+    typename std::iterator_traits<ForwardIt>::difference_type count = last - first;


This type is referenced for defining step also. A type alias could be created.

Yes I will update

science-enthusiast · 2025-01-27T13:20:03Z

include/ddc/storage_discrete_domain.hpp

+bool binary_search(ForwardIt first, ForwardIt last, const T& value, Compare comp)
+{
+    first = ::ddc::detail::lower_bound(first, last, value, comp);
+    return (!(first == last) && !(comp(value, *first)));


If we are sufficiently careful with how lower_bound and comp are implemented, could just !(first == last) be enough?

Same as lower_bound, this is a copy paste of the version found on cppref

I had a closer look at std::binary_search and std::lower_bound and understand better now. I was wrong in my previous comment. Both !(first == last) and !(comp(value, *first)) are needed. lower_bound searches for the first element in the partitioned range [first, last) which is not ordered before value. lower_bound can return last only if bool(comp(*iter, value)) is true for all iters. Both lower_bound and binary_search assume that [first, last) is partitioned. Also comp behaves like < and not <=. If for any element elem of [first, last), if bool(comp(elem, value)) does not imply !bool(comp(value, elem)) then the behavior is undefined. Hence the first element for which bool(comp(*iter, value)) is false should be either equal or greater than value.

Ok thanks a lot for the explanation!

tpadioleau · 2025-01-30T15:41:01Z

@science-enthusiast do you think it can be merged as is ?

science-enthusiast · 2025-01-30T16:10:17Z

@science-enthusiast do you think it can be merged as is ?

Yes, I think it can be merged. So that people can start using it. I have a few doubts that I will ask you. For me, this has been a good way to go deeper into the DDC code base. In the future, I will be able to make deeper comments.

science-enthusiast

The DDC code base is new to me and I continue to understand it better by reviewing this PR. It is better to merge these changes and let people start using it. Issues can be addressed in the future.

tpadioleau · 2025-01-30T16:50:53Z

The DDC code base is new to me and I continue to understand it better by reviewing this PR. It is better to merge these changes and let people start using it. Issues can be addressed in the future.

Yes thank you for your time.

tpadioleau self-assigned this Dec 5, 2024

tpadioleau changed the title ~~Remove optimization trick with mdspan~~ Sliced domains Dec 5, 2024

tpadioleau force-pushed the draft-sliced-domains branch from d42ad0e to b18c085 Compare December 5, 2024 21:48

tpadioleau changed the title ~~Sliced domains~~ Generalized domains Dec 10, 2024

tpadioleau force-pushed the draft-sliced-domains branch 3 times, most recently from 9196b14 to 5d8ac57 Compare December 13, 2024 19:40

tpadioleau mentioned this pull request Dec 13, 2024

Create a SubDomain #438

Closed

tpadioleau force-pushed the draft-sliced-domains branch 2 times, most recently from f17f86f to bc58e5c Compare December 16, 2024 10:47

tpadioleau force-pushed the draft-sliced-domains branch 4 times, most recently from b734484 to e2b02c6 Compare December 26, 2024 17:15

science-enthusiast reviewed Dec 29, 2024

View reviewed changes

include/ddc/chunk_common.hpp Show resolved Hide resolved

include/ddc/discrete_domain.hpp Show resolved Hide resolved

science-enthusiast reviewed Dec 31, 2024

View reviewed changes

include/ddc/discrete_domain.hpp Show resolved Hide resolved

tpadioleau force-pushed the draft-sliced-domains branch 2 times, most recently from 6b779a1 to adaa00b Compare January 2, 2025 13:14

tpadioleau force-pushed the draft-sliced-domains branch 7 times, most recently from 64ca9f7 to f297957 Compare January 16, 2025 17:46

tpadioleau marked this pull request as ready for review January 16, 2025 17:46

tpadioleau force-pushed the draft-sliced-domains branch 3 times, most recently from a7b6d26 to 360ed44 Compare January 25, 2025 09:06

tpadioleau force-pushed the draft-sliced-domains branch from 360ed44 to b9eecc3 Compare January 25, 2025 09:15

tpadioleau added 12 commits January 25, 2025 10:15

Remove internal_mdspan

a134e66

Transfer distance function in DiscreteDomain

6a1e802

Almost implement Chunk and ChunkSpan with SupportType

325a054

Add StridedDiscreteDomain

5d4187f

Add support for generalized domains in parallel_for_each and transfor…

78a3fa2

…m_reduce

Add support for generalized domains in for_each and transform_reduce

dcd3357

Add StorageDiscreteDomain class

99097ae

Reintroduce ddc::DiscreteDomain specialization and fix some warnings

1046fe4

Rename is_inside to contains

e084547

Enable some commented code

afaf006

Support for discrete element slicing

2478464

Support remove_dims_of

ce3cda4

tpadioleau force-pushed the draft-sliced-domains branch from b9eecc3 to ce3cda4 Compare January 25, 2025 09:16

science-enthusiast reviewed Jan 27, 2025

View reviewed changes

science-enthusiast approved these changes Jan 30, 2025

View reviewed changes

tpadioleau merged commit 18e3e55 into main Jan 30, 2025
57 checks passed

tpadioleau deleted the draft-sliced-domains branch January 30, 2025 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalized domains #698

Generalized domains #698

tpadioleau commented Dec 5, 2024 •

edited

Loading

tpadioleau commented Jan 16, 2025 •

edited

Loading

science-enthusiast Jan 27, 2025

tpadioleau Jan 27, 2025

science-enthusiast Jan 27, 2025 •

edited

Loading

tpadioleau Jan 27, 2025

science-enthusiast Jan 27, 2025

tpadioleau Jan 27, 2025

science-enthusiast Jan 27, 2025

tpadioleau Jan 27, 2025

science-enthusiast Jan 30, 2025

tpadioleau Jan 30, 2025

tpadioleau commented Jan 30, 2025

science-enthusiast commented Jan 30, 2025

science-enthusiast left a comment

tpadioleau commented Jan 30, 2025

Generalized domains #698

Generalized domains #698

Conversation

tpadioleau commented Dec 5, 2024 • edited Loading

tpadioleau commented Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

science-enthusiast Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpadioleau commented Jan 30, 2025

science-enthusiast commented Jan 30, 2025

science-enthusiast left a comment

Choose a reason for hiding this comment

tpadioleau commented Jan 30, 2025

tpadioleau commented Dec 5, 2024 •

edited

Loading

tpadioleau commented Jan 16, 2025 •

edited

Loading

science-enthusiast Jan 27, 2025 •

edited

Loading