Skip to content

EIG combine existing MKL and non-existing SYCL to one kernel #112

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
Merged
merged 1 commit into from
Oct 11, 2020
Merged

EIG combine existing MKL and non-existing SYCL to one kernel #112

merged 1 commit into from
Oct 11, 2020

Conversation

densmirn
Copy link
Contributor

@densmirn densmirn commented Oct 5, 2020

No description provided.

@densmirn densmirn requested a review from shssf October 5, 2020 11:55
@codecov
Copy link

codecov bot commented Oct 5, 2020 •
edited
Loading

Codecov Report

Merging #112 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #112   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          19      19           
  Lines        1240    1242    +2     
  Branches      329     330    +1     
======================================
- Misses       1240    1242    +2
Impacted Files Coverage Δ
dpnp/linalg/linalg_iface.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68815e3...b867789. Read the comment docs.

shssf
shssf reviewed Oct 5, 2020
View reviewed changes
dpnp/backend/custom_kernels.cpp Outdated
}
else
{
// TODO: implement SYCL kernel for int/long input
Copy link
Contributor

Choose a reason for hiding this comment

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

must throw an exception

shssf
shssf reviewed Oct 5, 2020
View reviewed changes
dpnp/backend/backend_iface_fptr.cpp Outdated
fmap[DPNPFuncName::DPNP_FN_EIG][eft_LNG][eft_LNG] = {eft_DBL, (void*)mkl_lapack_syevd_c<double>};
fmap[DPNPFuncName::DPNP_FN_EIG][eft_FLT][eft_FLT] = {eft_FLT, (void*)mkl_lapack_syevd_c<float>};
fmap[DPNPFuncName::DPNP_FN_EIG][eft_DBL][eft_DBL] = {eft_DBL, (void*)mkl_lapack_syevd_c<double>};
fmap[DPNPFuncName::DPNP_FN_EIG][eft_INT][eft_INT] = {eft_DBL, (void*)custom_lapack_syevd_c<double>};
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't understand yet how it worked before. In double variant we provide a kernel to the user with int input? it should not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't worked before, eig function supports only float and double.
@densmirn added int and long types in map here
https://github.com/IntelPython/dpnp/pull/86/files#diff-cc4d5932a758a21f53df6d834b67eab0R313-R314
Copy-paste mistake, I think.

shssf
shssf reviewed Oct 5, 2020
View reviewed changes
examples/example7.cpp Outdated
@@ -28,7 +28,7 @@ int main(int, char**)
array[size * i + i] = i + 1;
}

mkl_lapack_syevd_c<double>(array, result, size);
custom_lapack_syevd_c<double>(array, result, size);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please change this example input datatypes to int (at line 14)?

shssf
shssf approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@shssf shssf left a comment

Choose a reason for hiding this comment

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

Don't spend more time on this code. I'll take care of it.

@densmirn
Copy link
Contributor Author

densmirn commented Oct 5, 2020

Don't spend more time on this code. I'll take care of it.

Got it, won't prepare patch for this.

@Alexander-Makaryev
Copy link
Contributor

@densmirn @shssf
Computation of eigenvectors and eigenvalues in integer types is like computation of square root when yoy expect that result will be integer.
Another example, when you use matrix multiplication you can expect that result will be integer if inputs are integer but in case of eig floating point result is expected.
If you want to add integer types as input for this function then just make a conversion to floating point types before computation.

@shssf shssf force-pushed the feature/eig_kernel branch from 4b0188f to 7bdf3f6 Compare October 11, 2020 14:19
@shssf shssf force-pushed the feature/eig_kernel branch from 7bdf3f6 to b867789 Compare October 11, 2020 14:21
@shssf shssf merged commit b26d624 into master Oct 11, 2020
@shssf shssf deleted the feature/eig_kernel branch October 11, 2020 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alexander-Makaryev Alexander-Makaryev Alexander-Makaryev left review comments

@shssf shssf shssf approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@densmirn @Alexander-Makaryev @shssf