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.

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

MKL fixes #4523

Closed
wants to merge 2 commits into from
Closed

MKL fixes #4523

wants to merge 2 commits into from

Conversation

gf712
Copy link
Member

@gf712 gf712 commented Feb 12, 2019

I am not sure what causes this issue, but somehow separating evaluation of unary functions from the main expression fixes the issue.

I did some debugging and it seems like for example the square root operation of a matrix (.cwiseSqrt()) makes its rows and cols attributes be 0, which then causes an assertion error when this matrix is used in another operation.

This only happens with MKL backend and is first reported in #3992

@gf712
Copy link
Member Author

gf712 commented Feb 12, 2019

If there are any Eigen experts around it would be nice to know what is going wrong here so we can file a ticket at Eigen. From what I can gather is that unary functions (power and sqrt) are not being properly evaluated lazily and have to be force evaluated for these code snippets to work.

In any case I can run all the tests fine with MKL now, I just get some minor issues with tolerance on my machine. there are some other issues but not related to Eigen stuff

same issue here. Unary function evaluation has to be forced otherwise the array shape is not passed along 
fixed indent
@@ -66,8 +66,11 @@ void CSOBI::fit_dense(CDenseFeatures<float64_t>* features)
MatrixXd M0 = cor(EX,int(m_tau[0]));
EigenSolver<MatrixXd> eig;
eig.compute(M0);
MatrixXd SPH = (eig.pseudoEigenvectors() * eig.pseudoEigenvalueMatrix().cwiseSqrt() * eig.pseudoEigenvectors ().transpose()).inverse();
MatrixXd spx = SPH*EX;
MatrixXd EVMsqrt = eig.pseudoEigenvalueMatrix().cwiseSqrt();
Copy link
Member

Choose a reason for hiding this comment

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

just asking, is this covered by any tests that would detect changing results?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be covered here. That's how I found these bugs!

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a buildbot for MKL backend btw?

@vigsterkr
Copy link
Member

vigsterkr commented Feb 14, 2019 via email

@gf712
Copy link
Member Author

gf712 commented Feb 14, 2019

Add one to azure :)

OK! Just for GCC should be fine right? Might need some help setting it up.

@vigsterkr
Copy link
Member

vigsterkr commented Feb 14, 2019 via email

@gf712 gf712 mentioned this pull request Feb 15, 2019
@vigsterkr
Copy link
Member

closing in favour of #4529
@gf712 right?

@vigsterkr vigsterkr closed this Feb 16, 2019
@gf712
Copy link
Member Author

gf712 commented Feb 16, 2019

Yup!

@gf712 gf712 deleted the mkl_fix branch February 28, 2019 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants