Skip to content

Add grain_size hyperparameter into EmpiricalCovariance and PCA algorithms #2492

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

Merged
merged 12 commits into from
Jun 18, 2025

Conversation

Vika-F
Copy link
Contributor

@Vika-F Vika-F commented May 26, 2025

  • cpu_grain_size hyperparameter was added to EmpiricalCovariance estimator.

  • cpu_grain_size and cpu_macro_block hyperparameters were added to PCA estimator.

  • gen_daal4py.py was modified to report the problem with clang-format installation on the setup stage.


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.
  • I have updated the documentation to reflect the changes or created a separate PR with update and provided its number in the description, if necessary.
  • 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.

Performance

not applicable

@Vika-F Vika-F added the enhancement New feature or request label May 26, 2025
@Vika-F
Copy link
Contributor Author

Vika-F commented May 28, 2025

/intelci: run

Copy link

codecov bot commented May 30, 2025

Codecov Report

Attention: Patch coverage is 56.66667% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
onedal/decomposition/pca.cpp 40.00% 0 Missing and 9 partials ⚠️
onedal/covariance/covariance.cpp 0.00% 0 Missing and 4 partials ⚠️
Flag Coverage Δ
azure 79.76% <54.54%> (-0.08%) ⬇️
github 73.54% <53.33%> (-0.07%) ⬇️

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

Files with missing lines Coverage Δ
onedal/common/hyperparameters.py 76.36% <100.00%> (+1.36%) ⬆️
onedal/decomposition/pca.py 90.00% <100.00%> (+0.34%) ⬆️
sklearnex/decomposition/pca.py 91.09% <100.00%> (+0.09%) ⬆️
onedal/covariance/covariance.cpp 52.94% <0.00%> (-2.83%) ⬇️
onedal/decomposition/pca.cpp 42.58% <40.00%> (-0.28%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Vika-F
Copy link
Contributor Author

Vika-F commented Jun 2, 2025

/intelci: run

@Vika-F Vika-F marked this pull request as ready for review June 2, 2025 11:19
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 relatively ready to go, code is easy to read and follows convention. Rather than using lambdas, you can reference the hyperparameter type's methods directly.

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.

I reran the timed-out windows CI job, and make sure to finish the checklist. I think with a private CI run and @david-cortes-intel 's point addressed we can approve this.

"-style=file:" + formatfile,
]
)
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for doing this. Its one of those hidden things for development that catches me from time to time. Nitpick would be to add a note in the PR description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I also have problems with it from time to time. Decided to put an end to those :)
I've updated the description.

@Vika-F
Copy link
Contributor Author

Vika-F commented Jun 18, 2025

/intelci: run

@Vika-F Vika-F merged commit b203abd into uxlfoundation:main Jun 18, 2025
29 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants