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

Add sklearnex.BasicStatistics API for CSR inputs on GPU and a test for it #2253

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Add sklearnex.BasicStatistics API for CSR inputs on GPU and a test for it #2253

wants to merge 18 commits into from

Conversation

Vika-F
Copy link
Contributor

@Vika-F Vika-F commented Jan 13, 2025 •
edited
Loading

  • sklearnex.BasicStatistics API was extended to accept CSR inputs on GPU
  • The test for CSR inputs on GPU were added

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.

@Vika-F Vika-F mentioned this pull request Jan 13, 2025
Closed
13 tasks
@codecov Codecov
Copy link

codecov bot commented Jan 15, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 57.14286% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
sklearnex/basic_statistics/basic_statistics.py 50.00% 6 Missing ⚠️
Flag Coverage Δ
azure 77.97% <57.14%> (-0.07%) ⬇️
github 71.03% <57.14%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
onedal/basic_statistics/basic_statistics.py 100.00% <100.00%> (ø)
sklearnex/basic_statistics/basic_statistics.py 87.50% <50.00%> (-6.35%) ⬇️

@Vika-F
Copy link
Contributor Author

Vika-F commented Jan 15, 2025

/intelci: run

@Vika-F Vika-F changed the title Add sklearnex.BasicStatistics API for CSR inputs and a test for it Add sklearnex.BasicStatistics API for CSR inputs on GPU and a test for it Jan 16, 2025
@Vika-F Vika-F marked this pull request as ready for review January 16, 2025 09:14
@Vika-F Vika-F removed the request for review from samir-nasibli January 16, 2025 09:15
icfaust
icfaust reviewed Jan 16, 2025
View reviewed changes
Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Generally looks good, just some small stylistic things. It looks to function properly from my understanding.

onedal/basic_statistics/tests/utils.py Outdated Show resolved Hide resolved
sklearnex/basic_statistics/basic_statistics.py Show resolved Hide resolved
sklearnex/basic_statistics/tests/test_basic_statistics.py Outdated Show resolved Hide resolved
@Vika-F
Copy link
Contributor Author

Vika-F commented Jan 17, 2025

/intelci: run

icfaust
icfaust reviewed Jan 17, 2025
View reviewed changes
Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

The nitpick doesn't need to be done, but the change in tolerances in general should be changed.

sklearnex/basic_statistics/tests/test_basic_statistics.py Outdated Show resolved Hide resolved
sklearnex/basic_statistics/tests/test_basic_statistics.py Outdated Show resolved Hide resolved
sklearnex/basic_statistics/basic_statistics.py Show resolved Hide resolved
onedal/basic_statistics/tests/utils.py Outdated Show resolved Hide resolved
onedal/basic_statistics/tests/utils.py Outdated Show resolved Hide resolved
@Vika-F Vika-F mentioned this pull request Jan 20, 2025
Merged
6 tasks
Vika-F added 2 commits January 20, 2025 03:40
@Vika-F
1. Update the test after fixing the bug in min, max and sum_squares c…
2f2f77f 
…omputations in oneDAL; 2. Update options_and_tests dict to use unbiased statistics, alligned with oneDAL
@Vika-F
Fix failures in public CI
0ebfe4f
@Vika-F
Merge pull request #10 from uxlfoundation/main
dc0ff7c 
Pull the changes from main
@Vika-F
Copy link
Contributor Author

Vika-F commented Jan 21, 2025

/intelci: run

icfaust
icfaust reviewed Jan 24, 2025
View reviewed changes
Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Looks pretty close to me, just some subtle stuff and some things we should make tickets for (as future work).

onedal/basic_statistics/basic_statistics.py Show resolved Hide resolved
sklearnex/basic_statistics/basic_statistics.py Show resolved Hide resolved
onedal/basic_statistics/tests/utils.py Show resolved Hide resolved
sklearnex/basic_statistics/tests/test_basic_statistics.py Outdated Show resolved Hide resolved
sklearnex/basic_statistics/tests/test_basic_statistics.py Outdated Show resolved Hide resolved
@icfaust icfaust mentioned this pull request Jan 24, 2025
Open
13 tasks
@icfaust
Copy link
Contributor

icfaust commented Jan 24, 2025

/intelci: run

icfaust
icfaust approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

Merge when CI's show green 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-cortes-intel david-cortes-intel david-cortes-intel left review comments

@icfaust icfaust icfaust approved these changes

@Alexsandruss Alexsandruss Awaiting requested review from Alexsandruss Alexsandruss is a code owner

Assignees
No one assigned
Labels
gpu_interfaces
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Vika-F @icfaust @david-cortes-intel