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

For the transposition directive in gemv, CblasConjTrans just gives results as CblasTrans. #54

Open
shengg opened this issue Jul 17, 2014 · 16 comments
Labels

Comments

@shengg
Copy link
Contributor

shengg commented Jul 17, 2014

For blas functions, gemv and gemm, there are parameters to determine how to transpose for the matrix: NoTrans, Trans or ConjTrans. In btas, all of these three parameters are implemented for gemm. However, only NoTrans and Trans are implemented for gemv. If ConjTrans is used, it is considered as Trans. I do not know why.

@shiozaki
Copy link
Contributor

I think complex support is not well implemented (I guess I was the only person who uses complex arithmetics?), so I would expect that it is simply a bug.

@naokin
Copy link
Contributor

naokin commented Jul 18, 2014

That should be a bug.
But, as long as calling CBLAS, I think they work correctly, calling with
TensorView might have a trouble.

On Fri, Jul 18, 2014 at 6:54 AM, Toru Shiozaki [email protected]
wrote:

I think complex support is not well implemented (I guess I was the only
person who uses complex arithmetics?), so I would expect that it is simply
a bug.


Reply to this email directly or view it on GitHub
#54 (comment).

@evaleev
Copy link
Member

evaleev commented Jul 18, 2014

@shengg Most contract-like and permute should be assumed to NOT work with
TensorView. Although TensorView meets the TWG.Tensor concept spec (checked
by btas::is_boxtensor), and hence gets accepted by genv() and like, most of
these functions assume "contiguous" view. Assume that most uses of
TensorView will produce wrong results and should be checked.

All, note pull request #53 that addresses the issue of const TensorView not
behaving like TensorConstView. The only way I found this to be solvable is
through a configurable parameter (hence a distinct type of TensorView).
Feel free to comment. test.C has the relevant examples that explain the new
TensorRWView.

On Thu, Jul 17, 2014 at 8:29 PM, Naoki Nakatani [email protected]
wrote:

That should be a bug.
But, as long as calling CBLAS, I think they work correctly, calling with
TensorView might have a trouble.

On Fri, Jul 18, 2014 at 6:54 AM, Toru Shiozaki [email protected]
wrote:

I think complex support is not well implemented (I guess I was the only
person who uses complex arithmetics?), so I would expect that it is
simply
a bug.


Reply to this email directly or view it on GitHub
#54 (comment).


Reply to this email directly or view it on GitHub
#54 (comment).

web: valeyev.net

@evaleev evaleev added bug and removed question labels Jul 18, 2014
@shiozaki
Copy link
Contributor

The unit test is now broken (with Commit: bde5b24). @shengg Is this related to this issue?

test is a Catch v1.0 b48 host application.
Run with -? for options
-------------------------------------------------------------------------------
Tensor Gemv
  Complex Double Gemv --- ConjTrans
-------------------------------------------------------------------------------
tensor_blas_test.cc:223
...............................................................................

tensor_blas_test.cc:371: FAILED:
  CHECK( res < 1E-10 )
with expansion:
  121.1902981253 < 0.0000000001

===============================================================================
14 test cases - 1 failed (2370 assertions - 1 failed)

Makefile:29: recipe for target 'run' failed
gmake: *** [run] Error 1

@shengg
Copy link
Contributor Author

shengg commented Jul 22, 2014

Yes.
In gemv_impl.h, there are only TransA == CblasNoTrans and TransA != CblasNoTrans. There is no option to distinguish CblasConjTrans from CblasTrans .

@shengg
Copy link
Contributor Author

shengg commented Jul 22, 2014

@shiozaki I made a small change. It works now.

@shiozaki
Copy link
Contributor

Thanks - but if _HAS_CBLAS is defined, it still breaks.

@shengg
Copy link
Contributor Author

shengg commented Jul 23, 2014

I used mkl cblas. When _HAS_CBLAS is defined, several other tests also failed.

@shiozaki
Copy link
Contributor

I do use MKL CBLAS on Mac, and this (tensor_blas_test.cc:371) is the only test that fails.

@shengg
Copy link
Contributor Author

shengg commented Jul 23, 2014

I should check my MKL or try another version. Maybe, it was not configured well, since MKL in my desktop was never used before.

@shiozaki
Copy link
Contributor

For what it's worth, my MKL is composer_xe_2013_sp1.0.074 and I do in ~/.profile

source /opt/intel/bin/compilervars.sh intel64

@shiozaki
Copy link
Contributor

WAIT! you are right, there are some tests that are broken (I was looking at a wrong build). I will try to look into it as far as I can.

@shiozaki
Copy link
Contributor

There are bugs in tests due to hardwired threshold. Note below that float does not have 10 digits accuracy. I think they should be implemented using std::numerical_limits::epsilon() in some way.

 48     SECTION("Float Dot")
 49         {
 50         Tensor<float> Tf(2,3,7,3);
 51         Tf.generate([](){ return randomReal<float>(); });
 52         const auto fres = dot(Tf,Tf);
 53         float fcheck = 0.;
 54         for(const auto& el : Tf) fcheck += el*el;
 55         CHECK(fabs(fcheck-fres) < 1E-10);

@shengg
Copy link
Contributor Author

shengg commented Jul 23, 2014

Yes, It should be changed. When I did not define _HAS_CBLAS, it works for
some tests. So, I only changed the accuracy for the ones that failed.

However, after defining _HAS_CBLAS, the deviation was very big. And some
tests for double also failed.

Best regards,

Sheng Guo
Ph.D student
352 Frick Laboratory
Department of Chemistry
Princeton University

On Wed, Jul 23, 2014 at 12:08 PM, Toru Shiozaki [email protected]
wrote:

There are bugs in tests due to hardwired threshold. Note below that float
does not have 10 digits accuracy. I think they should be implemented using
std::numerical_limits::epsilon() in some way.

48 SECTION("Float Dot")
49 {
50 Tensor Tf(2,3,7,3);
51 Tf.generate({ return randomReal(); });
52 const auto fres = dot(Tf,Tf);
53 float fcheck = 0.;
54 for(const auto& el : Tf) fcheck += el*el;
55 CHECK(fabs(fcheck-fres) < 1E-10);


Reply to this email directly or view it on GitHub
#54 (comment).

@shiozaki
Copy link
Contributor

In addition, do not use fabs in the global scope from math.h. They are only implemented for double, so if you use it for float, it involves casts that introduce numerical noise.

The numerical behavior _HAS_CBLAS mainly stems from loop optimization (and use of SSE/AVX instructions) and is legitimate.

@shiozaki
Copy link
Contributor

They are only -> They may be only. Anyway it seems to change the behavior on my laptop. I guess it is better to use std::abs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants