PMR^3: templated eigensolver for Elemental #189

mcopik · 2016-10-21T23:34:15Z

This pull request introduces a templated version of PMR^3. It is a result of student project at RWTH from last winter semester and it has been already discussed on mailing list in February.
Our results generated with several tridiagonal matrices of different types (Legendre, Wilkinson, matrices obtained from chemical problems) show a decrease of computation time between 20 and 50%, when desired accuracy allows for single precision computations. We didn't notice any performance change for double-precision computation after switching from pure C to C++.

I've been working with the original PMR^3 repository to create a templatized version and then I applied changes from Elemental's version of PMR^3. Significant changes between C and C++ versions enforced a rather manual process of patching the eigensolver.

PMR^3 requires two additional preprocessor flags (pthreads and spinlock) and to avoid polluting Elemental with additional flags, a config file for PMR^3 is created by CMake and placed in both build and install directory.

There are few issues to resolve; I'm also not quite sure about coding style and properly defining imports for templated PMR^3. All suggestions and comments are welcomed.

Issues:

In Elemental's version code for sorting eigenvalues has been removed, but a corresponding preprocessor flag in pmrrr.h is still there (ASSERT_SORTED_EIGENPAIRS), only the default value has been changed to false. It seems to me that sorting has been moved to Elemental. Should I apply those changes to new version and remove additional sorting?
Are Fortran prototypes still necessary?
global.h in PMR^3 handles different versions of C, including lack of support for standard C. Is it safe to assume that we have a decent support for C++ (at least C++03/11)? I could remove all those unnecessary definitions.

CC: @pauldj

jeffhammond · 2016-10-21T23:45:59Z

global.h in PMR^3 handles different versions of C, including lack of support for standard C. Is it safe to assume that we have a decent support for C++ (at least C++03/11)? I could remove all those unnecessary definitions.

Elemental assumed C++11 in late 2013 according to my email archives.

rhl- · 2016-10-22T01:23:57Z

I'm not sure exactly how you've made changes here, but, it appears that you are "moving" files. I would strongly recommend making use of git mv as it helps make the review process easier. that way git shows a more intelligent diff.

mcopik · 2016-10-22T01:31:08Z

@jeffhammond thanks!
@rhl I might have made the mistake of manually moving files in the beginning. Recently I have been using git mv all the time.

mcopik · 2016-10-22T01:34:05Z

There is insane amount of warnings in Clang build caused by C code translated from Fortran. Maybe it's worth fixing, because they will appear in compilation of each translation unit.

poulson · 2016-10-22T01:37:16Z

I haven't had a chance to step through this yet, but this is a much needed addition. And I fully agree that it would be better to not ever use f2c. My recent experience with implementing tridiagonal and bidiagonal divide and conquer natively in Elemental shows that starting from the bottom up is not as much work as one would think.

poulson · 2016-10-22T05:04:52Z

external/pmrrr/include/pmrrr/blas/odcpy.hpp

@@ -0,0 +1,115 @@
+/* odcpy.f -- translated by f2c (version 20061008) */


Wouldn't it be easier to just have a for loop instead of calling odcpy?

poulson · 2016-10-22T05:05:52Z

external/pmrrr/include/pmrrr/blas/odscal.hpp

@@ -0,0 +1,107 @@
+/**


Same for this routine. I vote for the obvious two-line trivial implementation.

poulson · 2016-10-22T05:08:56Z

external/pmrrr/include/pmrrr/lapack/odnst.hpp

+
+namespace pmrrr { namespace lapack {
+
+	template<typename FloatingType>


This routine seems like low-hanging fruit for a ground-up implementation as well; also, how will the fabs function for non-standard datatypes (e.g., El::BigFloat)?

Good point, to be honest I haven't paid much attention to non-standard datatypes.

Is it fine that I use existing MPI and math functions wrappers from Elemental? Or do you prefer for the PMR^3 to be independent on Elemental? I can implement simple serialization there if it is necessary.

In an ideal world, it would be part of the main library and use proper C++ functions instead of f2c, but the latter may take a substantial amount of work. You may want to look at how I handled similar issues in ElSuiteSparse in external/suite_sparse.

mcopik · 2016-10-22T15:31:06Z

@poulson Templated eigensolver is not new, it is still based on existing PMR^3 implementation which did use f2c extensively. I agree that the code is horrible to read and some changes may be necessary. For example, detection of character encoding in olsame could be replaced only with tools from standard library.

jedbrown · 2016-10-22T20:38:29Z

@rhl- @mcopik For what it's worth, git mv doesn't do anything special. It is semantically identical to adding the new file and removing the old one. The Git file format doesn't even have a way to state that a file was "moved". Instead, Git (log, diff, etc.) computes a similarity index when deciding whether to report a file as being new versus renamed. I haven't looked at this case, but a character encoding change would make a file look totally different (all lines changed). Anyway, you can control the similarity threshold:

       -M[<n>], --find-renames[=<n>]
           Detect renames. If n is specified, it is a threshold on the similarity index
           (i.e. amount of addition/deletions compared to the file’s size). For example,
           -M90% means Git should consider a delete/add pair to be a rename if more than
           90% of the file hasn’t changed. Without a % sign, the number is to be read as a
           fraction, with a decimal point before it. I.e., -M5 becomes 0.5, and is thus
           the same as -M50%. Similarly, -M05 is the same as -M5%. To limit detection to
           exact renames, use -M100%. The default similarity index is 50%.

poulson · 2016-10-29T14:19:43Z

examples/lapack_like/HermitianEig.cpp

-typedef double Real;
-typedef Complex<Real> C;
+template<typename Real>
+void run_example(Int n, bool print)


It isn't properly documented, but the examples/ folder is meant to be more for demonstrating functionality, and the tests/ folder is meant for correctness tests.

As an aside, Elemental uses CamelCase for function names rather than snake_case.

It might also be preferred to test both single-precision and double-precision in the same run (with the ideal case being able to individually disable each precision).

Thanks for the comment, I'm going to fix that.

By being able to disable each precision you mean a compilation flag? Or runtime argument?

poulson · 2016-10-29T14:21:39Z

external/pmrrr/CMakeLists.txt

 option(HAVE_SPINLOCKS "Enable if pthread lib supports spinlocks" OFF)
 MARK_AS_ADVANCED(HAVE_SPINLOCKS)
 if(NOT HAVE_SPINLOCKS)
-  add_definitions(-DNOSPINLOCKS)
+  set(pmrrr_defines "${pmrrr_defines}#define NOSPINLOCKS\n")


What is the reason for this change?

Those defines have been used as a flag to build PMR^3 as a library. Now most of the code (and 100% of code relying on availability of pthreads and spinlocks) have been moved to templated code included by Elemental. To make configuration flags available, I changed one of PMR^3 headers to a CMake configuration file, installed in both build and install directory.

That's why those flags are accumulated and used for configuration of the header, not passed directly to compiler.

Thank you for the detailed reply; though it seems it would be more usual to use cmakedefine within the configure file rather than explicit string includes.

Thanks! I didn't know about that feature.

mcopik and others added 30 commits November 9, 2015 17:34

Create config file for CMake

685bf4a

Fix the wrong passing of GFORTRAN_LIB to Scalapack

e94630b

Merge remote branch 'upstream/master'

dc13004

Merge branch 'master' of github.com:mcopik/Elemental

ca7c9e7

Fix passing incorrect include/link strings to ParMETIS

628436f

Merge remote branch 'upstream/master'

724df2d

Merge remote branch 'upstream/master'

27da86b

Merge remote-tracking branch 'upstream/master'

3854e23

New PMRRR

bcc2999

Adding new PMRRR

08c8a0c

Update PMRRR

f2f6b08

Update file not compiling with PMRRR

fb90124

Add example

c26ff4e

Remove unnecessary config file

7e216cd

Merge upstream/master

a52aa80

Fix include for PMR3

cd42d3b

Update single-precision eigensolver example

47dec0c

Remove wrong merge

01d2037

Apply Elemental's changes to PMR3 plarre

7cf5d11

Apply Elemental's changes to PMR3 plarrv

0480119

Apply Elemental's changes to PMR3 process_c_task

bdbce36

Apply Elemental's changes to PMR3 tasks

6d35bb6

Apply Elemental's changes to PMR3

6bb4639

PMR3: fix doubles introduced by a mistake

9d125f7

PMR3: additional API change for tasks.hpp

f31bfa7

PMR3: apply changes for definition headers

9a3b3a7

PMR3: remove unnecesary header file

8268f91

PMR3: remove unnecesary header file

98a618b

PMR3: fix converting a string to char*

1b7acb9

PMR3: change name of odscl

25d68e8

mcopik added 11 commits October 21, 2016 00:56

PMR3: apply final changes

fd4c0d3

Remove dead code

f0981a0

PMR3: fix double destructon of lock

cada3ad

PMR3: create a header file with definitions

38157c4

PMR3: remove last dependencies on doubles

aeb7cbb

Merge upstream/master

e98005d

PMR3: modify HermitianEig example

2c625f6

PMR3: disable sorting

997a039

PMR3: remove debug output

c8b3d4b

PMR3: remove unused double precision import for PMR3

edd0c45

PMR3: remove debug output

3beb5fc

PMR3: Fix incorrect header guard

66400aa

mcopik added 2 commits October 22, 2016 03:57

PMR3: remove warnings in lsame

b66876e

PMR3: remove warnings in lapack code

39cdaa0

poulson reviewed Oct 22, 2016

View reviewed changes

poulson reviewed Oct 29, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PMR^3: templated eigensolver for Elemental #189

PMR^3: templated eigensolver for Elemental #189

mcopik commented Oct 21, 2016

jeffhammond commented Oct 21, 2016

rhl- commented Oct 22, 2016

mcopik commented Oct 22, 2016

mcopik commented Oct 22, 2016

poulson commented Oct 22, 2016

poulson Oct 22, 2016

poulson Oct 22, 2016

poulson Oct 22, 2016

mcopik Oct 22, 2016

poulson Oct 22, 2016

mcopik commented Oct 22, 2016 •

edited

Loading

jedbrown commented Oct 22, 2016

poulson Oct 29, 2016

mcopik Oct 29, 2016

poulson Oct 29, 2016

mcopik Oct 29, 2016

poulson Oct 29, 2016

mcopik Oct 29, 2016

		@@ -0,0 +1,115 @@
		/* odcpy.f -- translated by f2c (version 20061008) */


		namespace pmrrr { namespace lapack {

		template<typename FloatingType>

PMR^3: templated eigensolver for Elemental #189

Are you sure you want to change the base?

PMR^3: templated eigensolver for Elemental #189

Conversation

mcopik commented Oct 21, 2016

jeffhammond commented Oct 21, 2016

rhl- commented Oct 22, 2016

mcopik commented Oct 22, 2016

mcopik commented Oct 22, 2016

poulson commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcopik commented Oct 22, 2016 • edited Loading

jedbrown commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcopik commented Oct 22, 2016 •

edited

Loading