8000 Bug in Backend.OpenBLAS.fs · Issue #11 · DiffSharp/DiffSharp · GitHub
[go: up one dir, main page]
Skip to content

Bug in Backend.OpenBLAS.fs #11

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
Closed
ghost opened this issue Dec 13, 2015 · 8 comments
Closed

Bug in Backend.OpenBLAS.fs #11

ghost opened this issue Dec 13, 2015 · 8 comments
Labels
bug

Comments

@ghost
Copy link
ghost commented Dec 13, 2015

Hallo,

At first congratulations to this library, but I've found two little bugs in module LAPACK in function ssysv and dsysv, which make these functions useless.
You must correct line 487 from: use arg_b = new PinnedArray(b)
to use arg_b = new PinnedArray(b')
and similar (variable b -> b') in line 580.

Enrico
@gbaydin
Copy link
Member
gbaydin commented Dec 13, 2015

Hi! Thank you for reporting this. I will look into it and let you know.

@ghost
Copy link
Author
ghost commented Dec 13, 2015

Ok, if you look deeper into the OpenBLAS module then there is (maybe?) another issue.
In my opinion the matrices in the DM module are in row major order, but LAPACK assume per default column major order. This is not a problem if you've only symmetric matrices, but in all other cases.
If this is really an issue, then you have three possible solutions.

  1. DiffSharp use per default column major order
  2. swap the indexers
  3. use the LAPACKE interface (if OpenBLAS has one), then you can define the order. But this can eventually slowdown the calculations

@mrakgr
Copy link
Contributor
mrakgr commented Dec 14, 2015

I've checked into that and currently there is a bug in the latest release of OpenBLAS that prevents the LAPACKE function from being used in row major format. I've worked around that by adding manual transposes and solved these issues in the latest pull request.

@brada4
Copy link
brada4 commented Dec 14, 2015

It is bug in LAPACKE, you can check with generic LAPACK 3.6.0

@gbaydin gbaydin added the bug label Dec 14, 2015
@gbaydin
Copy link
Member
gbaydin commented Dec 14, 2015

I can confirm that the bugs you pointed out in ssysv and dsysv caused incorrect return values. This is now fixed in the repo.

@gbaydin
Copy link
Member
gbaydin commented Dec 14, 2015

Regarding @grek142's mention of another issue about LAPACK row- and column-major order:

I do not see any bug related with that.

.NET arrays are stored in memory using row-major order and the library will continue using these. We are aware of LAPACK's way of working with matrices and we designed our wrapper code paying attention to this.

To make sure that we have a better way of catching similar bugs in future, I have now implemented unit tests for the backend interface. Everything with the OpenBLAS backend seems to be working fine, including operations that depend on LAPACK, such as SolveSymmetric_M_V, Inverse_M, and Det_M.

If you encounter a bug where any operation gives incorrect result, please do let us know by opening a separate issue for that, showing a minimal example producing incorrect output.

Also, I have not experienced any unexpected behavior with the LAPACK implementation in OpenBLAS.

@ghost
Copy link
Author
ghost commented Dec 14, 2015

Ok, thank You. As You said this was just a mention not really a bug in the library.
(I just saw the problem as I'd wrote my own wrapper for LAPACK to work with the raw data of a hessian matrix. For simplicity I'd used the dposv and dpotri methods and this was not the best choice with the benefit of hindsight.)

@ghost ghost closed this as completed Dec 14, 2015
@gbaydin
Copy link
Member
gbaydin commented Dec 14, 2015

Thank you. :)

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gbaydin @mrakgr @brada4
0