gonum / blas

A BLAS implementation for Go [DEPRECATED]
172 stars 16 forks source link

Fix and clean up comments in cblas64/128 #154

Closed vladimir-ch closed 8 years ago

vladimir-ch commented 8 years ago

I'm confused. Why does the build fail due to missing sqrt and why has the coverage increased?

kortschak commented 8 years ago

I don't have time to look into this for the next few days. Both things are mysterious.

kortschak commented 8 years ago

Has coverage increased because total line count has decreased?

vladimir-ch commented 8 years ago

I don't get the coveralls UI, so I cannot confidently interpret it. My command line says:

$ wc -l cblas128/cblas128.go cblas64/cblas64.go 
  510 cblas128/cblas128.go
  510 cblas64/cblas64.go
 1020 total

$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.

$ wc -l cblas128/cblas128.go cblas64/cblas64.go
  497 cblas128/cblas128.go
  497 cblas64/cblas64.go
  994 total
kortschak commented 8 years ago

The link failure isn't repeatable on my machine here, but a restart on travis shows it is there. Presumably libmath is not being specified somewhere that we were getting for free previously.

vladimir-ch commented 8 years ago

I tried to restart a rebuild of master and now it is failing too, previously it wasn't. Shall I try to add -lm to gonum/blas/.travis/linux/OpenBLAS/install.sh?

kortschak commented 8 years ago

I guess it's worth a try. Please take a look at the failure on Brendan's dsterf PR in lapack and see if that failure is the same - I restarted one to see the log (travis doesn't like this machine for unknowable reasons) and it passed, but the rest failed.

vladimir-ch commented 8 years ago

That failure is also from the linker but the reason is different, many errors about undefined references to LAPACKE functions.

kortschak commented 8 years ago

And it doesn't fail for me here either. I'll try replicating on my workstation tomorrow. It's running archaic ubuntu too, so I might be able to repeat there.

vladimir-ch commented 8 years ago

Adding -lm seems promising, the branch openblas-lm is all green on travis. Submitting PR.

kortschak commented 8 years ago

LGTM

Presumably you will rebase this onto the new master with -lm?

vladimir-ch commented 8 years ago

The build passes, no rebase is necessary. Merging.