Change Point, Box, Sphere CTAD #1145

aprokop · 2024-09-13T23:31:14Z

Allow CTAD only for floating point types without any promotion.

aprokop · 2024-09-14T02:33:41Z

Will rebase on #1100 once merged.

dalg24 · 2024-09-14T12:12:46Z

What is the rational behind promoting integral types to float instead of double like it is done in the standard library math functions?

dalg24 · 2024-09-14T12:13:17Z

Also why should we enable mixing types rather than being strict?

aprokop · 2024-09-14T15:45:34Z

What is the rational behind promoting integral types to float instead of double like it is done in the standard library math functions?

Originally I was thinking performance as operations with floats on GPUs are much faster than with doubles.

However, you asking the questions makes me think. Not all integers are represented with floats. Even fewer long ints are represented with floats. So maybe that's why the standard library converts integers to doubles by default, as all 32-bit integers can be represented by doubles.

At the same time, not all 64-bit integers are represented by doubles either.

I guess we need to decide when this utility is useful, and what assumptions go into it.
Removing it would require us to always having to cast, like Point{0.f, 1.f} or Point{i}.

Also why should we enable mixing types rather than being strict?

Mostly so that Point{.3f, 0} would work.

aprokop · 2024-09-18T11:16:46Z

Passed the tests. Warnings are unrelated.

dalg24 · 2024-09-23T15:24:32Z

test/tstCompileOnlyGeometry.cpp

@@ -228,19 +228,38 @@ void test_point_cv_compile_only()
  static_assert(GT::is_point_v<Point const>);
 }

-void test_point_ctad()
+void test_geometry_ctad()


I'd slightly prefer having separate functions for the different types.

test/tstCompileOnlyGeometry.cpp

dalg24 · 2024-09-23T20:09:03Z

src/geometry/ArborX_Box.hpp

+KOKKOS_FUNCTION
+#endif
+Box(T const (&)[N], T const (&)[N])
+    -> Box<N, std::conditional_t<std::is_integral_v<T>, double, T>>;


Suggested change

-> Box<N, std::conditional_t<std::is_integral_v<T>, double, T>>;

-> Box<N, std::enable_if_t<std::is_integral_v<T>, double>>;

Not tested but it feels like we would potentially enable deductions we did not mean to with your version

I think you version would not deduce Box{{float}, {float}}.

Not tested but it feels like we would potentially enable deductions we did not mean to with your version

Which ones?

I thought we were only enabling CTAD for arithmetic types. Is that not what the intent was?

I had a dual intent: enable conversion for arithmetic types, and allow CTAD for a single type.

I have mixed feelings about the promotion of integral types if all other types are treated differently and we don't spell out any constraint.

I think I understand your concern. However, I would not want Point(0, 0, 0) to produce Point<3, int>.

dalg24 · 2024-09-23T20:15:21Z

src/geometry/ArborX_Point.hpp

+#else
+KOKKOS_FUNCTION
+#endif
+Point(T, Ts...) -> Point<


Why do we have both T and Ts?

So that we could check that all types are identical. It feels easier than trying to get the first type from a parameter pack.

Why do we have both this guide and the array reference one?

I thought users may benefit from that one too. I don't have a strong need for it at the moment.

You mean not needing brace initialization?

I think users may expect both Point(a, b, c) and Point({a, b, c}) working.

masterleinad · 2024-09-24T19:46:13Z

Is there a good use cases for allowing Coordinate to be integral or should we just restrict to floating point types (and don't try to convert anything)?

aprokop · 2024-09-24T19:50:09Z

Is there a good use cases for allowing Coordinate to be integral or should we just restrict to floating point types (and don't try to convert anything)?

The only current use case is in our tests, like ArborX::Point{0, 0, 0}. Those would all require changes. Otherwise, I think not allowing integral types could be ok.

masterleinad · 2024-09-24T20:15:44Z

The only current use case is in our tests, like ArborX::Point{0, 0, 0}. Those would all require changes. Otherwise, I think not allowing integral types could be ok.

I think enforcing the the type of the constructor parameters to match and use as Coordinate would be the most straightforward then.

src/geometry/ArborX_Point.hpp

masterleinad · 2024-09-25T13:01:26Z

src/geometry/ArborX_Point.hpp

+             std::enable_if_t<std::is_floating_point_v<T> &&
+                                  (... && std::is_floating_point_v<Ts>)&&(
+                                      ... &&std::is_same_v<std::decay_t<T>,
+                                                           std::decay_t<Ts>>),
+                              T>>;


Suggested change

std::enable_if_t<std::is_floating_point_v<T> &&

(... && std::is_floating_point_v<Ts>)&&(

... &&std::is_same_v<std::decay_t<T>,

std::decay_t<Ts>>),

T>>;

std::enable_if_t<std::conjunction_v<std::is_same<T, Ts>...>, T>>;

seems to do the job just fine, see https://godbolt.org/z/hMM69qGPr. The error messages are better when the decution guide doesn't constraint to floating point types bu the class has a corresponding static_assert.

The error messages are better when the dedution guide doesn't constraint to floating point types but the class has a corresponding static_assert.

I don't think it's a good idea to put that static assert in the class. If a user really points to use Point<D,int>, we should let them do that.

Drop the decay and if Ts are the same as T and T is a floating point you don't need to check that Ts are floating points

I don't think it's a good idea to put that static assert in the class. If a user really points to use Point<D,int>, we should let them do that.

Why would we then restrict the deduction guide at all?

Why would we then restrict the deduction guide at all?

I think that Point{0,0,0} resulting in Point<3, int> may not be what most user expect. I'd rather have them explicitly write Point<3, int>{0, 0, 0}.

aprokop · 2024-09-30T15:37:55Z

@dalg24 @masterleinad Any objections to merging this?

masterleinad · 2024-09-30T15:43:11Z

@dalg24 @masterleinad Any objections to merging this?

I'm still not liking it too much that we restrict CTAD but not the class itself. Maybe, that really is a separate question but the error messages when trying CTAD with, say, int arguments aren't very good.

aprokop · 2024-09-30T21:39:01Z

OK. I made CTAD as simple as possible. It does not do any checks on the types anymore.

aprokop · 2024-10-01T00:19:01Z

OK, this version should be good enough. If we change our minds, we'll change it before the release.

aprokop marked this pull request as draft September 14, 2024 02:33

aprokop force-pushed the change_points_ctad branch from b6f9897 to d8f2d94 Compare September 16, 2024 18:41

aprokop mentioned this pull request Sep 17, 2024

Switch ArborX::Point to hyper-point #1100

Merged

aprokop force-pushed the change_points_ctad branch from d8f2d94 to 9f8e2a6 Compare September 17, 2024 20:23

aprokop marked this pull request as ready for review September 17, 2024 20:26

aprokop requested a review from dalg24 September 17, 2024 20:31

aprokop added the backward incompatible Modifications that may break users' code label Sep 22, 2024

aprokop force-pushed the change_points_ctad branch 4 times, most recently from 6fee055 to 07bcdb3 Compare September 23, 2024 16:24

aprokop mentioned this pull request Sep 23, 2024

Geometries switch tracker #1144

Closed

dalg24 reviewed Sep 23, 2024

View reviewed changes

aprokop changed the title ~~Change Point CTAD~~ Change Point, Box, Sphere CTAD Sep 25, 2024

masterleinad reviewed Sep 25, 2024

View reviewed changes

aprokop force-pushed the change_points_ctad branch from 9eb9c88 to ecb04b4 Compare September 25, 2024 14:04

Add CTAD for Point, Box, Sphere and fix things

2dadc8c

aprokop force-pushed the change_points_ctad branch from fa86f97 to 2dadc8c Compare September 30, 2024 21:38

masterleinad approved these changes Sep 30, 2024

View reviewed changes

aprokop merged commit 4dbf7e3 into arborx:master Oct 1, 2024
1 of 2 checks passed

aprokop deleted the change_points_ctad branch October 1, 2024 00:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Point, Box, Sphere CTAD #1145

Change Point, Box, Sphere CTAD #1145

aprokop commented Sep 13, 2024 •

edited

Loading

aprokop commented Sep 14, 2024

dalg24 commented Sep 14, 2024

dalg24 commented Sep 14, 2024

aprokop commented Sep 14, 2024

aprokop commented Sep 18, 2024

dalg24 Sep 23, 2024

dalg24 Sep 23, 2024

aprokop Sep 23, 2024 •

edited

Loading

dalg24 Sep 24, 2024

aprokop Sep 24, 2024

dalg24 Sep 24, 2024

aprokop Sep 24, 2024

dalg24 Sep 23, 2024

aprokop Sep 23, 2024

dalg24 Sep 24, 2024

aprokop Sep 24, 2024

dalg24 Sep 24, 2024

aprokop Sep 24, 2024

masterleinad commented Sep 24, 2024

aprokop commented Sep 24, 2024

masterleinad commented Sep 24, 2024

masterleinad Sep 25, 2024

aprokop Sep 25, 2024

dalg24 Sep 25, 2024

aprokop Sep 25, 2024

masterleinad Sep 25, 2024

aprokop Sep 25, 2024

aprokop commented Sep 30, 2024

masterleinad commented Sep 30, 2024

aprokop commented Sep 30, 2024

aprokop commented Oct 1, 2024 •

edited

Loading

	-> Box<N, std::conditional_t<std::is_integral_v<T>, double, T>>;
	-> Box<N, std::enable_if_t<std::is_integral_v<T>, double>>;

Change Point, Box, Sphere CTAD #1145

Change Point, Box, Sphere CTAD #1145

Conversation

aprokop commented Sep 13, 2024 • edited Loading

aprokop commented Sep 14, 2024

dalg24 commented Sep 14, 2024

dalg24 commented Sep 14, 2024

aprokop commented Sep 14, 2024

aprokop commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprokop Sep 23, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad commented Sep 24, 2024

aprokop commented Sep 24, 2024

masterleinad commented Sep 24, 2024

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

aprokop commented Sep 30, 2024

masterleinad commented Sep 30, 2024

aprokop commented Sep 30, 2024

aprokop commented Oct 1, 2024 • edited Loading

aprokop commented Sep 13, 2024 •

edited

Loading

aprokop Sep 23, 2024 •

edited

Loading

aprokop commented Oct 1, 2024 •

edited

Loading