-
Notifications
You must be signed in to change notification settings - Fork 217
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
initial commit for adding boost lib #2985
base: main
Are you sure you want to change the base?
initial commit for adding boost lib #2985
Conversation
const Float* data = s.get_data(); | ||
|
||
Eigen::Matrix<Float, Eigen::Dynamic, Eigen::Dynamic> eigen_matrix(row_count, column_count); | ||
for (int i = 0; i < eigen_matrix.rows(); ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An Eigen matrix can also be created from a non-owned pointer for float/double data. I think something along the lines of Eigen::map< Eigen::Matrix<info, incl. row/col major> , alignment, stride >
.
const la::matrix<Float>& eigvecs, | ||
const la::matrix<Float>& eigvals) const { | ||
INFO("convert results to float64"); | ||
const auto s_f64 = la::astype<double>(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative, perhaps the input values could be hard-coded along with the solutions instead of checking them against a different library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but with this solution there is no opportunity to use random generated data(I mean extend tests with for example row_count = GENERATE(3, 28, 125, 256);) and also it will be complicated to check results for big datasets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about putting them in the existing folders with .csv files that have data and expected results?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible, but let's say for pca it's necessary to contain gold eigenvectors, eigenvalues, and it will increase the total size of the repo, especially with big datasets. I see no reasons to avoid this pr tbh
Description
Add a comprehensive description of proposed changes
List associated issue number(s) if exist(s): #6 (for example)
Documentation PR (if needed): #1340 (for example)
Benchmarks PR (if needed): IntelPython/scikit-learn_bench#155 (for example)
PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.
You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance