Skip to content
This repository was archived by the owner on Nov 24, 2018. It is now read-only.

Conversation

@btracey
Copy link
Member

@btracey btracey commented Oct 6, 2015

No description provided.

@btracey
Copy link
Member Author

btracey commented Oct 6, 2015

Better documentation and more checks coming.

I'm not sure how to test this other than to rewrite the code again. However, this code will be called by a bunch of routines so it should be effectively tested then.

@btracey
Copy link
Member Author

btracey commented Oct 12, 2015

Good news: Just added tests for Dlasr
Bad news: It exposed a mismatch between the condition number computation for Dgecon and Dpocon that's significant (0.021 to 0.024 while all of the others match to 1e-14)
Worse news: Cgo and native give the same wrong answer.

Suggestions? Log a bug with OpenBLAS, and comment out the tests I've added for that case. What about the dpocon script itself? It's generating random cases at present.

@kortschak
Copy link
Member

Worse news: Cgo and native give the same wrong answer.

This is not good, but it's not terrible. At least the issue is consistent and code that work elsewhere works just as well here.

Suggestions? Log a bug with OpenBLAS, and comment out the tests I've added for that case.

I think so.

What about the dpocon script itself? It's generating random cases at present.

What do you mean?

@btracey
Copy link
Member Author

btracey commented Oct 12, 2015

The added case arises when generating the random test cases (at least with the random number stream on my machine). In order to pass the tests this case shouldn't be generated. I could, for example, make the loop shorter, but that dousn't feel like the right solution.

@kortschak
Copy link
Member

Maybe log cases >1e-14 and fail case >1e-2?

@btracey
Copy link
Member Author

btracey commented Oct 12, 2015

Filed OpenMathLib/OpenBLAS#664

@btracey
Copy link
Member Author

btracey commented Oct 13, 2015

@btracey
Copy link
Member Author

btracey commented Oct 13, 2015

Implemented suggestions.

native/dlasr.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

The docs in LAPACK are very informative. Could this be expanded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@kortschak
Copy link
Member

LGTM after test comment improved. I'll leave the doc comment up to you.

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 here?

btracey added a commit that referenced this pull request Oct 15, 2015
@btracey btracey merged commit 0d0a623 into master Oct 15, 2015
@vladimir-ch vladimir-ch deleted the adddlasr branch May 31, 2016 21:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants