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.

Sign up for GitHub

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

Jump to bottom

Adding correlation distance metric in oneDAL primitives #3059

Merged
merged 38 commits into from
Mar 6, 2025
Merged

Adding correlation distance metric in oneDAL primitives #3059

merged 38 commits into from
Mar 6, 2025

Conversation

richardnorth3
Copy link
Contributor

@richardnorth3 richardnorth3 commented Feb 3, 2025 •
edited
Loading

PR completeness and readability

  • I have reviewed my changes thoroughly before submitting this pull request.
  • I have commented my code, particularly in hard-to-understand areas.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have added a respective label(s) to PR if I have a permission for that.
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.
  • All CI jobs are green or I have provided justification why they aren't.
  • I have extended testing suite if new functionality was introduced in this PR.

@icfaust
Copy link
Contributor

icfaust commented Feb 3, 2025 •
edited
Loading

oneDAL/cpp/oneapi/dal/backend/primitives/reduction/reduction.hpp

Line 94 in 272269d

inline sycl::event reduce_by_columns(sycl::queue& q,

https://github.com/uxlfoundation/oneDAL/blob/main/cpp/oneapi/dal/backend/primitives/stat/cov.hpp#L34
should help you in calculating the means for each row (column_wise reduction to generate the sum, then calculate the mean using the second function)

this will leverage the GPUs capabilities

icfaust
icfaust reviewed Feb 14, 2025
View reviewed changes
cpp/oneapi/dal/backend/primitives/distance/metrics.hpp Outdated
ndview<Float, 2>& out,
const event_vector& deps = {}) const {
const std::int64_t n = u.get_dimension(0);
auto u_sum = ndarray<Float, 1>::empty({ 1 });
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for writing this up @richardnorth3 ! https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.distance.correlation.html should be a good reference for the means necessary. The mean doesn't need to be a single value, but a vector the size of a single sample with each being the mean value for each feature (i.e. for each row). This will then be a vector subtraction. Let me know if I am misunderstanding things about your implementation. Good progress.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey sorry about that, the definition is the mean of the 1d array, so my recommendations have been a little bit wrong.

@richardnorth3
Copy link
Contributor Author

richardnorth3 commented Feb 24, 2025 •
edited
Loading

https://github.com/richardnorth3/oneDAL/blob/9b09822dd84b8eb2ee555e08b4576c8ce7d9c0e2/cpp/oneapi/dal/backend/primitives/distance/correlation_distance_dpc.cpp#L50-L95

After heavily mediating over numerous drafts and codebase reviews, I ultimately came back to your original advice @icfaust after an epiphany. This version of the correlation distance reuses most of the cosine distance code but calculates the deviations first, passes those values to get_inversed_norms(), and proceeds with the same functionality as the cosine distance computation.

@richardnorth3 richardnorth3 added dpc++ Issue/PR related to DPC++ functionality oneAPI Issue/PR related to oneAPI part labels Mar 3, 2025
@Vika-F
Copy link
Contributor

Vika-F commented Mar 5, 2025

@Mergifyio rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Mar 5, 2025 •
edited by richardnorth3
Loading

rebase

☑️ Nothing to do

  • -conflict [📌 rebase requirement]
  • -closed [📌 rebase requirement]
  • queue-position = -1 [📌 rebase requirement]
  • any of:
    • #commits > 1 [📌 rebase requirement]
    • #commits-behind > 0 [📌 rebase requirement]
    • -linear-history [📌 rebase requirement]

richard.north.iii and others added 13 commits March 5, 2025 08:05
Adding correlation distance metric
283d033
@richardnorth3
Update metrics.hpp
43d3209
@richardnorth3
Update metrics.hpp with correlation_metric for GPU
09cd56a 
Signed-off-by: North Iii <richard.north.iii@intel.com>
@richardnorth3
Add correlation distance test
0789041 
Signed-off-by: North Iii <richard.north.iii@intel.com>
@richardnorth3
Add supporting files for correlation distance
0bd9b7b 
Signed-off-by: North Iii <richard.north.iii@intel.com>
@richardnorth3
Update distance.hpp with correlation prototypes
8bf5a66 
Signed-off-by: North Iii <richard.north.iii@intel.com>
@richardnorth3
Update metrics.hpp
e4d584b
@richardnorth3
Update metrics.hpp
134cfdc
@richardnorth3
Update correlation_distance_dpc.cpp
911e931 
Signed-off-by: North Iii <richard.north.iii@intel.com>
@richardnorth3
Update correlation_distance_dpc.cpp
04ea922 
Signed-off-by: North Iii <richard.north.iii@intel.com>
@richardnorth3
Update correlation_distance_dpc.cpp
ebdefd6 
Signed-off-by: North Iii <richard.north.iii@intel.com>
@richardnorth3
Update correlation_distance_dpc.cpp
e21845c 
Signed-off-by: North Iii <richard.north.iii@intel.com>
@richardnorth3
Update bazel BUILD file with cov deps
32df51e 
Signed-off-by: North Iii <richard.north.iii@intel.com>
@richardnorth3 richardnorth3 force-pushed the dev/correlation-metric branch from 4658d8f to 7c877d4 Compare March 5, 2025 16:16
Alexandr-Solovev
Alexandr-Solovev reviewed Mar 5, 2025
View reviewed changes
Copy link
Contributor

@Alexandr-Solovev Alexandr-Solovev left a comment

Choose a reason for hiding this comment

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

Overall, it looks good to me! Well done! I have a few minor questions and suggestions for now

@Alexandr-Solovev
Copy link
Contributor

/intelci: run

Apply changes from PR-3059 comments
fccf5ef 
Signed-off-by: richard.north.iii <rnorthii@smtp.jf.intel.com>
Update correlation_distance files
c0b188a 
Signed-off-by: richard.north.iii <rnorthii@smtp.jf.intel.com>
@richardnorth3
Copy link
Contributor Author

/intelci: run

Reapply clang-format to correlation_distance_dpc
96b4319 
Signed-off-by: richard.north.iii <rnorthii@smtp.jf.intel.com>
@Vika-F
Copy link
Contributor

Vika-F commented Mar 6, 2025

/intelci: run

Alexandr-Solovev
Alexandr-Solovev approved these changes Mar 6, 2025
View reviewed changes
Copy link
Contributor

@Alexandr-Solovev Alexandr-Solovev left a comment

Choose a reason for hiding this comment

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

The private ci in the commit before apply clang format is green! Well done!

Vika-F
Vika-F approved these changes Mar 6, 2025
View reviewed changes
Copy link
Contributor

@Vika-F Vika-F left a comment

Choose a reason for hiding this comment

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

The failure in latest CI is not related to the changes. LGTM.

@richardnorth3 richardnorth3 merged commit 2c316ac into uxlfoundation:main Mar 6, 2025
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icfaust icfaust

@Vika-F Vika-F

@Alexandr-Solovev Alexandr-Solovev

@Alexsandruss Alexsandruss

@samir-nasibli samir-nasibli

@napetrov napetrov

@homksei homksei

@ahuber21 ahuber21

@ethanglaser ethanglaser

@maria-Petrova maria-Petrova

Assignees
No one assigned
Labels
dpc++ Issue/PR related to DPC++ functionality oneAPI Issue/PR related to oneAPI part
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@richardnorth3 @icfaust @Vika-F @Alexandr-Solovev