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

Few missing arnoldi tests #22963

Merged
merged 3 commits into from
Jul 31, 2017
Merged

Few missing arnoldi tests #22963

merged 3 commits into from
Jul 31, 2017

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Jul 25, 2017

No description provided.

@kshyatt kshyatt added linear algebra Linear algebra test This change adds or pertains to unit tests labels Jul 25, 2017
@kshyatt kshyatt requested a review from andreasnoack July 25, 2017 23:51
@test norm(v) > testtol # eigenvectors cannot be null vectors
@test_warn "Use symbols instead of strings for specifying which eigenvalues to compute" eigs(a, which="LM")
@test a*v[:,2] ≈ d[2]*v[:,2]
@test norm(v) > testtol # eigenvectors cannot be null vectors
Copy link
Contributor

Choose a reason for hiding this comment

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

these 2 lines seem like exact duplicates, should there be a new d and v they operate on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, derrrrp on my part, sorry about that

@kshyatt
Copy link
Contributor Author

kshyatt commented Jul 26, 2017

Sorry about the bad whitespace. Good to go?

@@ -31,6 +31,10 @@ using Base.Test
(d,v) = eigs(a, nev=3)
@test a*v[:,2] ≈ d[2]*v[:,2]
@test norm(v) > testtol # eigenvectors cannot be null vectors
(d,v) = eigs(a, I, nev=3) # test two argument eigs
Copy link
Member

Choose a reason for hiding this comment

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

Slightly confused: three argument eigs? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nev is a kwarg.

Copy link
Member

Choose a reason for hiding this comment

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

A keyword argument is still an argument, no? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A second class argument!

(d,v) = eigs(a, I, nev=3) # test two argument eigs
@test a*v[:,2] ≈ d[2]*v[:,2]
@test norm(v) > testtol # eigenvectors cannot be null vectors
@test_warn "Use symbols instead of strings for specifying which eigenvalues to compute" eigs(a, which="LM")
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a thing? Shouldn't we deprecate the change from String -> Symbol for the which kwarg instead of this warning?
Unrelated to this PR though, I can submit a PR for the deprecation if thats the correct thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect it's a "protecting the fragile feelings of users" thing - is there an ARPACK wrapper that uses strings? I agree it's messed up but if the code there we should test it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the symbol is converted to a string before sent to the ARPACK wrapper, but still...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look, I'm just reporting the fact that this warning exists. ¯_(ツ)_/¯ I have no particular attachment to the question of whether it deserves to survive.

Copy link
Member

Choose a reason for hiding this comment

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

I know, just noticed it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole file is full of this kind of stuff haha

@kshyatt
Copy link
Contributor Author

kshyatt commented Jul 27, 2017

@Sacha0 are you ok with this? Ready to go?

@Sacha0
Copy link
Member

Sacha0 commented Jul 27, 2017

@Sacha0 are you ok with this? Ready to go?

I still find the comment confusing :).

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

:) !

@kshyatt kshyatt merged commit 5041126 into master Jul 31, 2017
@kshyatt kshyatt deleted the ksh/arnoldi branch July 31, 2017 18:59
@tkelman
Copy link
Contributor

tkelman commented Aug 1, 2017

looks like this should be reverted, it keeps failing on CI

tkelman added a commit that referenced this pull request Aug 1, 2017
This reverts commit 5041126.

This has been frequently failing on 64-bit Linux Travis with

```
Error in testset linalg/arnoldi:

Error During Test

  Test threw an exception of type Base.LinAlg.ARPACKException

  Expression: (eigs(speye(50), nev=10))[1] ≈ ones(10)

  Base.LinAlg.ARPACKException("unspecified ARPACK error: 3")
```
@kshyatt
Copy link
Contributor Author

kshyatt commented Aug 1, 2017

Sorry :(

@andreasnoack
Copy link
Member

I think this might be an issue with OpenBLAS. I was able to reproduce this on an Ivybridge machine running Linux. If I set OPENBLAS_CORETYPE="Nehalem" then the test failure goes away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants