Unit test method reuse

[ashermancinelli]

Many of the methods in the testing classes were simply copy/pasted into others as a shortcut, for example RAJA verify answer and CPU-bound verify answer. This shortcut will hold us back as we expand our testing framework.

[ashermancinelli]

@cameronrutherford is reviewing UT framework and will post his evaluation soon. From there we will act on the design changes.

[cameronrutherford]

When the unit test methods are currently re-used/duplicated, there are a few ways in which this happens:

1. The function is blatantly duplicated, and can easily be moved to a single implementation location.
2. The function is almost identical, but static casts the object first to a particular linear algebra object.
3. (2) occurs, but there is a call to copyFromDev before data is accessed.
4. (2) occurs, along with a call to getLocalElement instead of getLocalData.
5. The function is unique to the test, and should remain as such.

For cases 1-4 (which comprises as nearly all of our existing unit tests), the duplicated functions can be removed, and instead, we can implement a singular test function, that takes advantage of the abstract underlying base class of our linear algebra objects. This singular implementation can be defined in the associated base class, allowing child classes to call the same function. We can also take advantage of non-GPU objects having no-ops when the copyFromDev functions are called, allowing us to re-use the same tests for the RAJA and non-RAJA implementations.

There are only a handful of functions that fall under 5, and so these will most likely remain as is and cannot be consolidated.

It should be noted that sparse and dense matrices should still be treated differently, but keeping this in mind should still allow us to remove many of the duplicate methods. Given this, we should be able to move nearly all duplicated methods into the associated base testing classes by taking full advantage of the underlying base classes of our linear algebra objects.

In addition to these changes, @pelesh also mentioned that we should look an cleaning up our test drivers themselves. Per his suggestion, we could implement something along the lines of the following:

template <class TestT>
static int run_tests(std::string mem_space)
{
  int fail;
  options.SetStringValue("mem_space", mem_space);
  hiop::LinearAlgebraFactory::set_mem_space(options.GetString("mem_space"));
  std::string mem_space = hiop::LinearAlgebraFactory::get_mem_space();
  TestT test;
  // Initialize + run tests
  ...
  // Destroy testing objects
  ...
  // Set memory space back to default value
  options.SetStringValue("mem_space", "default");
  return fail;
}

By creating template functions for each of our unit test classes, we will be able to significantly clean up our test drivers. This will then hopefully allow us to sequentially test different memory spaces easily, along with ensuring that we apply the same set of unit tests across our various implementations.

I am sure that there will be much discussion on this topic moving forward, and there may be unforeseen difficulties with the approaches that I have outlined, but I think that this is a great starting point.

[ashermancinelli]

LGTM, as per our offline discussion with @pelesh. Please create a branch and start with moving some helper methods up the inheritance chain as you've explained. I suggest two stage approach so you can request review on your progress on each issue before opening PR for your main development branch for this issue.

[cameronrutherford]

Work is being done in branch unit-tests-dev, and PRs will be made to merge into that branch as progress is made.

[ashermancinelli]

Now that @cameronrutherford's work on vector tests has been merged in #141, we can begin working on refactoring matrix tests.

[cameronrutherford]

Current work on refactoring the dense matrix unit tests is being done in rcrutherford-matrix-unit-tests-dev.

@pelesh @ashermancinelli should we be testing the function matrixAddToSymDenseMatrixUpperTriangle? Currently the implementation for the test is completely commented out. Looking at the git blame, it appears to be a commit Cosmin made at some point during a PR.

One this is resolved, I can create a PR for this and we can move onto sparse matrices.

[ashermancinelli]

@cameronrutherford have you tried running the test? Does it pass? Is the kernel called anywhere in the optimization library?

[cameronrutherford]

The compiler has informed me that ‘class hiop::hiopMatrixDense has no member named addToSymDenseMatrixUpperTriangle, and so currently this is not a function that can be tested.

@ashermancinelli are there future plans to implement and test this function? If so, we can leave this in the code for now. Otherwise, it should probably be removed.

When looking into this, I also noticed specific casting to hiopMatrixDense * in test matrixTransAddToSymDenseMatrixUpperTriangle that seemed unnecessary. As a result, I removed it in this commit.

[pelesh]

are there future plans to implement and test this function? If so, we can leave this in the code for now. Otherwise, it should probably be removed.

No, this function was recently removed from the dense matrix interface. You can remove commented out code from unit tests.

[cnpetra]

closing this since it seems to have been addressed. please reopen if it's not the case.