Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port alpaka phi functions to DataFormats #142

Merged

Conversation

VourMa
Copy link
Collaborator

@VourMa VourMa commented Dec 27, 2024

This PR addresses the comments from our CMSSW integration PR where people were asking us to port the generic alpaka functions to CMSSW. I copied our LST functions to the package DataFormats/Math, under the interface/alpaka folder, which I thought it would be useful for people to add any additional math functions that have been written in alpaka.

The functions are not exact copies of what we have in

template <typename TAcc>
ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE float eta(TAcc const& acc, float x, float y, float z) {
float r3 = alpaka::math::sqrt(acc, x * x + y * y + z * z);
float rt = alpaka::math::sqrt(acc, x * x + y * y);
float eta = ((z > 0) - (z < 0)) * alpaka::math::acosh(acc, r3 / rt);
return eta;
}
template <typename TAcc>
ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE float phi_mpi_pi(TAcc const& acc, float x) {
if (alpaka::math::abs(acc, x) <= kPi)
return x;
constexpr float o2pi = 1.f / (2.f * kPi);
float n = alpaka::math::round(acc, x * o2pi);
return x - n * float(2.f * kPi);
}
template <typename TAcc>
ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE float phi(TAcc const& acc, float x, float y) {
return phi_mpi_pi(acc, kPi + alpaka::math::atan2(acc, -y, -x));
}
template <typename TAcc>
ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE float deltaPhi(TAcc const& acc, float x1, float y1, float x2, float y2) {
float phi1 = phi(acc, x1, y1);
float phi2 = phi(acc, x2, y2);
return phi_mpi_pi(acc, (phi2 - phi1));
}
template <typename TAcc>
ALPAKA_FN_HOST_ACC ALPAKA_FN_INLINE float deltaPhiChange(TAcc const& acc, float x1, float y1, float x2, float y2) {
return deltaPhi(acc, x1, y1, x2 - x1, y2 - y1);
}
ALPAKA_FN_ACC ALPAKA_FN_INLINE float calculate_dPhi(float phi1, float phi2) {
// Calculate dPhi
float dPhi = phi1 - phi2;
// Normalize dPhi to be between -pi and pi
if (dPhi > kPi) {
dPhi -= 2 * kPi;
} else if (dPhi < -kPi) {
dPhi += 2 * kPi;
}
return dPhi;
}

as I have tried to generalize, optimize and rename things a bit to be more inline with
https://github.com/SegmentLinking/cmssw/blob/master/DataFormats/Math/interface/deltaPhi.h.

Note that I have not ported the eta functions. As far as I checked and tested, it is not used in our code any longer, so it will be deleted in an upcoming PR. Also, calculate_dPhi can be replaced with

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

so it has also not been ported.

I have used the new functions in place of our current ones and things check out, as can be seen in:
https://uaf-10.t2.ucsd.edu/~evourlio/SDL/CMSSWPR142/compare/

Nothing much to test in this PR but feedback is welcome before I move this to cms-sw.

@VourMa
Copy link
Collaborator Author

VourMa commented Dec 27, 2024

/run all

@VourMa VourMa requested a review from slava77 December 27, 2024 20:04
Copy link

There was a problem while building and running in standalone mode. The logs can be found here.

Copy link

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

DataFormats/PortableMath/interface/deltaPhi.h Outdated Show resolved Hide resolved
DataFormats/PortableMath/interface/deltaPhi.h Outdated Show resolved Hide resolved
DataFormats/PortableMath/interface/deltaPhi.h Outdated Show resolved Hide resolved
DataFormats/PortableMath/interface/deltaPhi.h Outdated Show resolved Hide resolved
@ariostas
Copy link
Member

I updated the CI to 15_0_0_pre1, so now the standalone test works again. I'll think of a way to have the CI always pick the latest release.

@VourMa VourMa force-pushed the CMSSW_14_2_0_pre4_workflowsAndGeneralFunctions_squashed branch from e6c4da9 to 4bf107d Compare December 30, 2024 18:18
@VourMa
Copy link
Collaborator Author

VourMa commented Dec 30, 2024

/run all

Copy link

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     44.9    392.5    187.3    152.7    146.7    548.4    123.1    232.6    150.5      3.7    1982.4    1389.1+/- 382.8     525.0   explicit[s=4] (target branch)
   avg     44.9    393.7    188.0    151.3    147.7    546.7    121.5    229.3    150.3      3.2    1976.6    1385.0+/- 381.2     524.3   explicit[s=4] (this PR)

Copy link

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

@VourMa VourMa force-pushed the CMSSW_14_2_0_pre4_workflowsAndGeneralFunctions_squashed branch from 4bf107d to dbf0fbf Compare December 31, 2024 00:20
@slava77
Copy link

slava77 commented Dec 31, 2024

thanks for the updates. I don't have anything to add; perhaps time to make a PR upstream

Copy link
Member

@ariostas ariostas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few small comments that are pretty pedantic, but I feel like they could be brought up during the review.

Comment on lines 1 to 2
#ifndef DataFormats_Math_alpaka_deltaPhi_h
#define DataFormats_Math_alpaka_deltaPhi_h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it should be

Suggested change
#ifndef DataFormats_Math_alpaka_deltaPhi_h
#define DataFormats_Math_alpaka_deltaPhi_h
#ifndef DataFormats_Math_interface_alpaka_deltaPhi_h
#define 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this name too generic? Maybe reducePhiRange would be better

// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since CMSSW now uses C++20 you could use std::numbers::pi_v<T> or std::numbers::inv_pi_v<T>

@VourMa
Copy link
Collaborator Author

VourMa commented Dec 31, 2024

I left a few small comments that are pretty pedantic, but I feel like they could be brought up during the review.

I agree with all your comments. However, I made sure that the new file closely follows DataFormats/Math/interface/deltaPhi.h in naming conventions and variables to follow the path of least resistance during review. At this stage, I would propose the PR upstream as is, and you can bring up these points as comments there and I would fully support implementing them (also in the non-alpaka file, if need be) - I can also bring them up if you prefer so.

@ariostas
Copy link
Member

Ah okay, that makes sense. Then yeah, it makes more sense to leave it as it is for consistency.

@VourMa VourMa force-pushed the CMSSW_14_2_0_pre4_workflowsAndGeneralFunctions_squashed branch 3 times, most recently from c043336 to 93b9111 Compare January 13, 2025 09:49
@VourMa VourMa force-pushed the CMSSW_14_2_0_pre4_workflowsAndGeneralFunctions_squashed branch from 93b9111 to b8880cf Compare January 14, 2025 19:59
@VourMa VourMa merged commit a168eac into master Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants