Support block matrices in several BLAS1 routines #226
Conversation
…idt to support block matrices, and add tests for Hadamard, Dot, ColumnTwoNorms, and ColumnMaxNorms.
tests/blas_like/ColumnNorms.cpp
Outdated
| T expected = 0; | ||
| for (Int i = 0; i < A.LocalHeight(); ++i) { | ||
| T val = A.GetLocal(i, j); | ||
| expected += val * val; |
There was a problem hiding this comment.
Explicitly accumulating the squares and then square-rooting should be fine for random data but can lead to very low accuracy in extreme cases. There is the routine El::UpdateScaledSquare for accumulating the square of the two norm as the product of a scale and a scaled square that should generally be preferred (and which is used within the norm computation routines).
There was a problem hiding this comment.
My main goal there was to provide a simple baseline that ensures there's no major issues (e.g. block vs element distributions causing problems) while not just reimplementing the method that's being tested. If you think it would be better to switch to UpdateScaledSquare in this case, I can do that.
tests/blas_like/ColumnNorms.cpp
Outdated
| } | ||
| expected = mpi::AllReduce(expected, g.ColComm()); | ||
| expected = Sqrt(expected); | ||
| if (Abs(got - expected) > 1e-5) { |
There was a problem hiding this comment.
It would be more rigorous to use a bound of the form n * limits::Epsilon<El::Base<T>>() times a small constant (e.g., 10).
tests/blas_like/Dot.cpp
Outdated
| TestDot<float, ELEMENT>(m, n, g, print); | ||
| TestDot<float, BLOCK>(m, n, g, print); | ||
| TestDot<double, ELEMENT>(m, n, g, print); | ||
| TestDot<double, BLOCK>(m, n, g, print); |
There was a problem hiding this comment.
Why exclude complex tests and more precise datatypes (e.g., El::DoubleDouble, El::QuadDouble, El::Quad, and El::BigFloat)? I would be happy to add the appropriate flag guards for the higher-precision tests, but the complex arithmetic variants also working is crucial.
tests/blas_like/Dot.cpp
Outdated
| TestDot<float, BLOCK>(m, n, g, print); | ||
| TestDot<double, ELEMENT>(m, n, g, print); | ||
| TestDot<double, BLOCK>(m, n, g, print); | ||
| } catch (exception& e) { |
There was a problem hiding this comment.
Nit: Elemental has made the (perhaps dubious) choice of putting curly braces on their own line.
tests/blas_like/Hadamard.cpp
Outdated
| for (Int i = 0; i < A.LocalHeight(); ++i) { | ||
| T got = C.GetLocal(i, j); | ||
| T expected = A.GetLocal(i, j) * B.GetLocal(i, j); | ||
| if (Abs(got - expected) > 1e-6) { |
There was a problem hiding this comment.
Same comment here about using El::limits::Epsilon<El::Base<T>>() times a small constant.
tests/blas_like/Hadamard.cpp
Outdated
| TestHadamard<float, ELEMENT>(m, n, g, print); | ||
| TestHadamard<float, BLOCK>(m, n, g, print); | ||
| TestHadamard<double, ELEMENT>(m, n, g, print); | ||
| TestHadamard<double, BLOCK>(m, n, g, print); |
There was a problem hiding this comment.
Same comment here about testing the complex (and high-precision) cases.
| @@ -60,6 +60,9 @@ void Hadamard | |||
| LogicError("A, B, and C must share the same distribution"); | |||
| if( A.ColAlign() != B.ColAlign() || A.RowAlign() != B.RowAlign() ) | |||
| LogicError("A and B must be aligned"); | |||
| if ( A.BlockHeight() != B.BlockHeight() || | |||
There was a problem hiding this comment.
Thank you noticing this crucial portion of the extension!
…ision and in complex variants. Clean up the syntax and use limits::Epsilon for checking error.
|
My latest commit addresses your comments (except see the discussion on |
tests/blas_like/Dot.cpp
Outdated
| } catch (exception& e) { | ||
| TestDot<Complex<double>, ELEMENT>(m, n, g, print); | ||
| TestDot<Complex<double>, BLOCK>(m, n, g, print); | ||
| #if defined(EL_HAVE_QD) && defined(EL_ENABLE_DOUBLEDOUBLE) |
There was a problem hiding this comment.
There is no need for the EL_ENABLE macros here (or in any test driver), as they are only used to signal to some of the include macros used for template instantiation in the library.
tests/blas_like/ColumnNorms.cpp
Outdated
| T val = A.GetLocal(i, j); | ||
| expected += val * val; | ||
| } | ||
| expected = mpi::AllReduce(expected, g.ColComm()); | ||
| expected = Sqrt(expected); | ||
| if (Abs(got - expected) > 1e-5) { | ||
| if (Abs(got - expected) > 10 * limits::Epsilon<El::Base<T>>()) |
There was a problem hiding this comment.
The accumulated error should be a function of the number of entries (with said function being logarithmic if a tree-based scheme is used, but linear with the current single-process implementation): would you mind multiplying the right-hand side by the number of entries?
Also, it would be better to use a relative bound and to diving the left-hand side by the maximum of the norm and 1 (so that it still behaves well for zero norms).
There was a problem hiding this comment.
Just want to clarify: The new check should be Abs(got - expected) / std::max(expected, 1) > m * n * 10 * limits::Epsilon<El::Base<T>>()?
|
yes, if that isn't too much hassle. otherwise it is easy to think of cases where the check would fail. |
…umnTwoNorms test.
This adds support for block matrices in the
Hadamard,Dot,HilbertSchmidt,ColumnTwoNorms, andColumnMaxNormsfunctions (see #224). I also added tests forHadamard,Dot,ColumnTwoNorms, andColumnMaxNorms.Let me know if any additional changes are needed.