Port generic alpaka phi functions to `DataFormats/Math` #47033

VourMa · 2025-01-02T12:10:36Z

During the review of the LST integration PR #45117 it was identified that some of the math functions rewritten in the alpaka framework for usage in the LST algorithm could be made more widely available in CMSSW by porting them to a more central package:

This PR follows up on those comments. After the initial PR review, the choice made was to port these functions under Heterogeneous/AlpakaMath. The function logic is as close as possible to the non-alpaka equivalent DataFormats/Math/interface/deltaPhi.h file.

This PR only introduces the new function to CMSSW + the relevant unit tests. A follow-up PR will be made to actually use these functions in the LST algorithm and delete the LST-specific implementations.

cmsbuild · 2025-01-02T12:11:05Z

cms-bot internal usage

cmsbuild · 2025-01-02T12:13:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47033/43155

cmsbuild · 2025-01-02T12:13:59Z

A new Pull Request was created by @VourMa for master.

It involves the following packages:

DataFormats/Math (reconstruction)

@cmsbuild, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks.
@fabiocos, @felicepantaleo, @makortel, @missirol, @mmusich, @rovere this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

makortel · 2025-01-02T15:22:53Z

@cms-sw/heterogeneous-l2 (not sure if this PR would need to be assigned to heterogeneous as well)

The choice made was to port these functions under DataFormats/Math, creating a new folder, interface/alpaka but the exact placement could be discussed. The function logic is as close as possible to the non-alpaka equivalent DataFormats/Math/interface/deltaPhi.h file.

Thinking out loud

Considering package dependencies, making DataFormats/Math to depend on Alpaka would add an unnecessary dependence on Alpaka to nearly all packages that presently depend on DataFormats/Math (376 direct dependents)
- A proper dependence would need to be added to DataFormats/Math/BuildFile.xml
- From dependency standpoint e.g. HeterogeneousCore/AlpakaInterface would be a natural placement. We could also think of a new package, e.g. AlpakaMath.
Per https://github.com/cms-sw/cmssw/blob/master/HeterogeneousCore/AlpakaCore/README.md#overall-guidelines the header does not need to be in interface/alpaka, because it does not depend on ALPAKA_ACCELERATOR_NAMESPACE. The wording there doesn't preclude it though, although we have generally preferred to place such headers directly to interface directory.

makortel

I'd suggest to add unit tests.

DataFormats/Math/interface/alpaka/deltaPhi.h

makortel · 2025-01-02T15:26:34Z

DataFormats/Math/interface/alpaka/deltaPhi.h

+
+  // reduce to [-pi,pi]
+  template <typename TAcc, typename T>
+  ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE constexpr T reduceRange(TAcc const& acc, T x) {


I was going to comment the ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE are not necessary because the function is constexpr, but after looking at Alpaka implementation of e.g. abs() and round()
https://github.com/alpaka-group/alpaka/blob/a4142d3feb7686d803e1ec5f25d7b2278337f455/include/alpaka/math/Traits.hpp#L864
https://github.com/alpaka-group/alpaka/blob/a4142d3feb7686d803e1ec5f25d7b2278337f455/include/alpaka/math/Traits.hpp#L1331
that are not constexpr, I'm actually puzzled how this function can be constexpr.

@makortel when do we expect to switch to C++23? These functions will be constexpr in the standard/STL at that point.

Indeed, you are right. Compiling and using the functions did not show any errors though, is it that constexpr is silently ignored? If that's the case, I guess I should remove it so that it does not cause confusion.

when do we expect to switch to C++23?

Based on the experience with C++20, maybe around 2027.

Indeed, you are right. Compiling and using the functions did not show any errors though, is it that constexpr is silently ignored? If that's the case, I guess I should remove it so that it does not cause confusion.

It seems that constexpr functions and function templates are allowed to call non-constexpr functions as long as there is at least one set of arguments with which the function can be evaluated as a constant expression (and otherwise the program is ill-formed, no diagnostic required...).

From C++20 draft (N4849), 9.2.5.7 (.6 is also relevant)

If the instantiated template specialization of a constexpr function template or member function of a class template would fail to satisfy the requirements for a constexpr function, that specialization is still a constexpr function, even though a call to such a function cannot appear in a constant expression. If no specialization of the template would satisfy the requirements for a constexpr function when considered as a non-template function, the template is ill-formed, no diagnostic required.

So while our compilers might not warn about the situation, C++20-conforming code would not use constexpr here. Although IIUC this constraint was removed from C++23.

Since C++23 seems to be quite a bit away for CMSSW, I would go with the C++20 conforming option and remove the constexpr.

Note that ALPAKA_FN_INLINE is much stronger than inline, as it tries to force the compiler to actually inline the function.

The regular inline should be fine most of the time.

The regular inline should be fine most of the time.

Should I be switching to that in this case? I am not sure whether you were suggesting that I apply this change or it was a more general comment.

makortel · 2025-01-02T15:34:38Z

DataFormats/Math/interface/alpaka/deltaPhi.h

+  // reduce to [-pi,pi]
+  template <typename TAcc, typename T>
+  ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE constexpr T reduceRange(TAcc const& acc, T x) {
+    constexpr T o2pi = 1. / (2. * M_PI);


Now that we have C++20, how about

Suggested change

constexpr T o2pi = 1. / (2. * M_PI);

constexpr T o2pi = T{1.} / (T{2.} * std::numbers::pi_v<T>);

?

Is it intentional to do the calculations on double and cast the result to T?

Is T expected to be only a floating point type?

I think that it would be better to maintain the implementation as close as possible to the original deltaPhi.h

Indeed, the implementation here is a faithful adaptation of the code in

cmssw/DataFormats/Math/interface/deltaPhi.h

Lines 17 to 24 in 2f2cacb

template <typename T>

constexpr T reduceRange(T x) {

constexpr T o2pi = 1. / (2. * M_PI);

if (std::abs(x) <= T(M_PI))

return x;

T n = std::round(x * o2pi);

return x - n * T(2. * M_PI);

}

Looking at the following lines in that file

cmssw/DataFormats/Math/interface/deltaPhi.h

Lines 26 to 32 in 2f2cacb

constexpr double deltaPhi(double phi1, double phi2) { return reduceRange(phi1 - phi2); }

constexpr double deltaPhi(float phi1, double phi2) { return deltaPhi(static_cast<double>(phi1), phi2); }

constexpr double deltaPhi(double phi1, float phi2) { return deltaPhi(phi1, static_cast<double>(phi2)); }

constexpr float deltaPhi(float phi1, float phi2) { return reduceRange(phi1 - phi2); }

I suppose that the intention is that T is only a floating point type but I am not sure. For LST purposes (for which these functions are going to be used, at least initially), floating point type only T is enough.

That said, your suggestion makes sense but maybe it should be propagated to interface/deltaPhi.h as well? Not sure whether we want to do so though (it was not the initial purpose of this PR).

That said, your suggestion makes sense but maybe it should be propagated to interface/deltaPhi.h as well? Not sure whether we want to do so though (it was not the initial purpose of this PR).

Ok, maybe it would make sense to keep both versions consistent, and maybe any changes would be beyond this PR.

I suppose that the intention is that T is only a floating point type but I am not sure. For LST purposes (for which these functions are going to be used, at least initially), floating point type only T is enough.

I was wondering if anyone would be interested to add that as a type constraint either as

template <typename TAcc, typename T> requires std::is_floating_point_v<T> ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE T reduceRange(TAcc const& acc, T x) {

or

template <typename TAcc> ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE std::floating_point auto reduceRange(TAcc const& acc, std::floating_point auto x) {

but probably that's beyond this PR as well.

Note that doing the computation in double precision may be quite expensive on most GPUs. For example it is 64 times more expensive than single precision on an NVIDIA L4.

I will go ahead and apply the suggestion by Matti to fix this:
constexpr T o2pi = T{1.} / (T{2.} * std::numbers::pi_v<T>);

slava77 · 2025-01-02T18:34:37Z

Considering package dependencies, making DataFormats/Math to depend on Alpaka would add an unnecessary dependence on Alpaka to nearly all packages that presently depend on DataFormats/Math (376 direct dependents)
* A proper dependence would need to be added to `DataFormats/Math/BuildFile.xml`

is there a way to keep the dependence only via /alpaka directory includes? ... I guess the question goes in the direction of having it in a separate directory, effectively.

makortel · 2025-01-02T23:06:14Z

Considering package dependencies, making DataFormats/Math to depend on Alpaka would add an unnecessary dependence on Alpaka to nearly all packages that presently depend on DataFormats/Math (376 direct dependents)
* A proper dependence would need to be added to `DataFormats/Math/BuildFile.xml`
is there a way to keep the dependence only via /alpaka directory includes? ... I guess the question goes in the direction of having it in a separate directory, effectively.

No. For link-time dependencies that wouldn't work. Some source-level operations, like IIRC git cms-checkdeps -a, operate based on what files have been modified.

VourMa · 2025-01-03T12:44:14Z

Thinking out loud

Considering package dependencies, making DataFormats/Math to depend on Alpaka would add an unnecessary dependence on Alpaka to nearly all packages that presently depend on DataFormats/Math (376 direct dependents)

A proper dependence would need to be added to DataFormats/Math/BuildFile.xml

From dependency standpoint e.g. HeterogeneousCore/AlpakaInterface would be a natural placement. We could also think of a new package, e.g. AlpakaMath.

Per https://github.com/cms-sw/cmssw/blob/master/HeterogeneousCore/AlpakaCore/README.md#overall-guidelines the header does not need to be in interface/alpaka, because it does not depend on ALPAKA_ACCELERATOR_NAMESPACE. The wording there doesn't preclude it though, although we have generally preferred to place such headers directly to interface directory.

Based on this and the follow up discussion, I understand that the current placement is not ideal, since the dependencies cannot be reduced. During our internal review, I had this initially placed under a new DataFormats subpackage, so I revert and put it under DataFormats/AlpakaMath/interface (i.e. also dropping the alpaka directory).

VourMa · 2025-01-03T13:12:03Z

I'd suggest to add unit tests.

Sure. Do you have in mind some unit test that I can use as a template (to speed up the work and to make sure that I cover what needs to be tested)?

slava77 · 2025-01-03T15:05:29Z

Based on this and the follow up discussion, I understand that the current placement is not ideal, since the dependencies cannot be reduced. During our internal review, I had this initially placed under a new DataFormats subpackage, so I revert and put it under DataFormats/AlpakaMath/interface (i.e. also dropping the alpaka directory).

IIUC, HeterogeneousCore/ subsystem was mentioned earlier by Matti, not DataFormats.

VourMa · 2025-01-03T15:08:09Z

Based on this and the follow up discussion, I understand that the current placement is not ideal, since the dependencies cannot be reduced. During our internal review, I had this initially placed under a new DataFormats subpackage, so I revert and put it under DataFormats/AlpakaMath/interface (i.e. also dropping the alpaka directory).

IIUC, HeterogeneousCore/ subsystem was mentioned earlier by Matti, not DataFormats.

Ah, sorry, I misinterpreted the comment to mean that the new subpackage would be under DataFormats. It's clear now, thanks.

slava77 · 2025-01-03T15:13:15Z

Ah, sorry, I misinterpreted the comment to mean that the new subpackage would be under DataFormats. It's clear now, thanks.

I guess @makortel could confirm the preference to avoid further confusion.

makortel · 2025-01-03T15:39:04Z

I'd suggest to add unit tests.

Sure. Do you have in mind some unit test that I can use as a template (to speed up the work and to make sure that I cover what needs to be tested)?

By quick look I see the existing reco::deltaPhi() and reco::reduceRange() being used in assertive way in

cmssw/DataFormats/Math/test/testAtan2.cpp

Lines 187 to 189 in f55e6a5

    
           ok(deltaPhi(phi1, phi2), int2phi(ip1 - ip2)); 
        
           ok(deltaPhi(phi2, phi1), int2phi(ip2 - ip1)); 
        
           ok(toPhi(phi2 + phi1), int2phi(ip1 + ip2));

(and even there it is not immediately clear whether reduceRange() or int2phi() is the one being tested).

I'd say the primary objective would be to ensure all functions can be called for the relevant data types (I suppose both float and double) can be used in the kernel code for all backends, and ideally at least for some inputs the functions return expected values (trying to cover all corner cases is a potential rabbit hole though).

Here are a few rather simple Alpaka-using unit tests

I personally prefer the Catch2 unit test framework (used in testAtomicPairCounter.dev.cc), but that is not required.

makortel · 2025-01-03T15:53:13Z

Ah, sorry, I misinterpreted the comment to mean that the new subpackage would be under DataFormats. It's clear now, thanks.

I guess @makortel could confirm the preference to avoid further confusion.

I'd wait for next week before moving things around to get more feedback (after more people will presumably be back)

I would not be against DataFormats/AlpakaMath, but I'd ask the questions

What is the probability these functions will be used in DataFormats code?
- If guessed to be at most tiny, the package could well be placed in other subsystem to avoid placing non-DataFormat code in DataFormats
What is the probability there will be more Alpaka-based math algorithms?
- If guessed at most tiny, perhaps an existing package would be less of a burden than a new package for one header
- If guessed to be above tiny (I could imagine this to be the case), a new package could be justified

DataFormats/Math/interface/alpaka/deltaPhi.h

VourMa · 2025-01-07T19:17:13Z

I would not be against DataFormats/AlpakaMath, but I'd ask the questions

What is the probability these functions will be used in DataFormats code?

If guessed to be at most tiny, the package could well be placed in other subsystem to avoid placing non-DataFormat code in DataFormats

My estimation is that it's tiny, so I will not go with that option.

What is the probability there will be more Alpaka-based math algorithms?

If guessed at most tiny, perhaps an existing package would be less of a burden than a new package for one header

If guessed to be above tiny (I could imagine this to be the case), a new package could be justified

I cannot really predict and I don't have a use case right now, but I can see people needing, e.g., deltaR in Alpaka code sooner than later. Based on that, I would go ahead and make a new AlpakaMath package. I will do so tomorrow, unless someone comes up with a strong preference.

cmsbuild · 2025-01-08T20:11:10Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47033/43221

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47033/43221/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47033/43221/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2025-01-08T20:28:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47033/43223

cmsbuild · 2025-01-08T20:28:33Z

Pull request #47033 was updated. @cmsbuild can you please check and sign again.

slava77 · 2025-01-10T14:39:33Z

@cmsbuild please test

I'm not sure why this was left unattended from the review side for two days

fwyzard · 2025-01-10T16:46:05Z

I'm not sure why this was left unattended from the review side for two days

On my side, because I am busy with other tasks.

fwyzard · 2025-01-10T16:47:52Z

HeterogeneousCore/AlpakaMath/interface/deltaPhi.h

+  // reduce to [-pi,pi]
+  template <typename TAcc, typename T>
+    requires std::is_floating_point_v<T>
+  ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE T reduceRange(TAcc const& acc, T x) {


reduceRange is not a good name, because it doesn't convey any relation to [-π, π].

maybe reducePhiRange would be better ?

I agree, and this was brought up by Andres as well during our internal discussion. However, in this PR, I tried to stick to the current CMSSW convention which uses reduceRange in multiple places for keeping φ within [-π, π]:
https://github.com/search?q=repo%3Acms-sw%2Fcmssw%20reduceRange&type=code
Do we want to change just for this version of this function?

Maybe a global renaming to something clearer (reducePhiRange sounds good to me) would make sense. If at least some of us think it's useful and it doesn't turn out to be a rabbithole, I could try to propose it in a different PR for wider review. But I wouldn't connect it to this PR.

cmsbuild · 2025-01-10T16:55:49Z

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4e8216/43721/summary.html
COMMIT: c043336
CMSSW: CMSSW_15_0_X_2025-01-10-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/47033/43721/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 1 lines from the logs
Reco comparison results: 12 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3818780
DQMHistoTests: Total failures: 115
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3818645
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 214 log files, 184 edm output root files, 49 DQM output files
TriggerResults: no differences found

slava77 · 2025-01-10T17:34:14Z

@fwyzard
should this be formally assigned to heterogenous? the PR currently has only orp atm

cmsbuild added this to the CMSSW_15_0_X milestone Jan 2, 2025

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending code-checks-pending labels Jan 2, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 2, 2025

makortel reviewed Jan 2, 2025

View reviewed changes

VourMa commented Jan 6, 2025

View reviewed changes

DataFormats/Math/interface/alpaka/deltaPhi.h Outdated Show resolved Hide resolved

VourMa force-pushed the CMSSW_14_2_0_pre4_workflowsAndGeneralFunctions_squashed branch from dbf0fbf to 99036e0 Compare January 8, 2025 20:08

cmsbuild added new-package-pending code-checks-pending and removed reconstruction-pending code-checks-approved labels Jan 8, 2025

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jan 8, 2025

Port alpaka phi functions to HeterogeneousCore

c043336

VourMa force-pushed the CMSSW_14_2_0_pre4_workflowsAndGeneralFunctions_squashed branch from 99036e0 to c043336 Compare January 8, 2025 20:25

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jan 8, 2025

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 8, 2025

cmsbuild added tests-started and removed tests-pending labels Jan 10, 2025

fwyzard reviewed Jan 10, 2025

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Jan 10, 2025

	constexpr T o2pi = 1. / (2. * M_PI);
	constexpr T o2pi = T{1.} / (T{2.} * std::numbers::pi_v<T>);

	template <typename T>
	constexpr T reduceRange(T x) {
	constexpr T o2pi = 1. / (2. * M_PI);
	if (std::abs(x) <= T(M_PI))
	return x;
	T n = std::round(x * o2pi);
	return x - n * T(2. * M_PI);
	}

	constexpr double deltaPhi(double phi1, double phi2) { return reduceRange(phi1 - phi2); }

	constexpr double deltaPhi(float phi1, double phi2) { return deltaPhi(static_cast<double>(phi1), phi2); }

	constexpr double deltaPhi(double phi1, float phi2) { return deltaPhi(phi1, static_cast<double>(phi2)); }

	constexpr float deltaPhi(float phi1, float phi2) { return reduceRange(phi1 - phi2); }

Port generic alpaka phi functions to DataFormats/Math #47033

Are you sure you want to change the base?

Port generic alpaka phi functions to DataFormats/Math #47033

Conversation

VourMa commented Jan 2, 2025 • edited Loading

cmsbuild commented Jan 2, 2025 • edited Loading

cmsbuild commented Jan 2, 2025

cmsbuild commented Jan 2, 2025

makortel commented Jan 2, 2025 • edited Loading

makortel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 Jan 6, 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

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

slava77 commented Jan 2, 2025

makortel commented Jan 2, 2025

VourMa commented Jan 3, 2025

VourMa commented Jan 3, 2025

slava77 commented Jan 3, 2025

VourMa commented Jan 3, 2025

slava77 commented Jan 3, 2025

makortel commented Jan 3, 2025

makortel commented Jan 3, 2025

VourMa commented Jan 7, 2025

cmsbuild commented Jan 8, 2025

cmsbuild commented Jan 8, 2025

cmsbuild commented Jan 8, 2025

slava77 commented Jan 10, 2025

fwyzard commented Jan 10, 2025

fwyzard Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VourMa Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

cmsbuild commented Jan 10, 2025

Comparison Summary

slava77 commented Jan 10, 2025

Port generic alpaka phi functions to `DataFormats/Math` #47033

Port generic alpaka phi functions to `DataFormats/Math` #47033

VourMa commented Jan 2, 2025 •

edited

Loading

cmsbuild commented Jan 2, 2025 •

edited

Loading

makortel commented Jan 2, 2025 •

edited

Loading

slava77 Jan 6, 2025 •

edited

Loading

fwyzard Jan 10, 2025 •

edited

Loading

VourMa Jan 10, 2025 •

edited

Loading