API for hiopMatrixSparseTriplet has implementation specific arguments about hiop HOT 9 CLOSED

I see a potential need to include such methods in an interface...

This would make testing and transitioning to new implementations much easier as you mentioned. Ideally every internal method deals with only an interface so we can swap out implementations as needed. In my opinion, making hiopMatrixSparseTriplet and hiopMatrixDense pure virtual would be the single most helpful step we could take in this regard.

from hiop.

that is intended design at least with the mentioned method(s) and for the time being -- only sparse matrices should have and need that functionality

from hiop.

CC: @cameronrutherford @JuanDiegoMontoya

from hiop.

I see a potential need to include such methods in an interface (for the sparse matrices) when porting, but it really depends on the specific of the device implementation, for example this would/should not be needed (ideally) if the hardware abstraction layer can run cpu or gpu in a way that is oblivious to the code using this class... like in RAJA vector class.

from hiop.

I should have clarified that using matrix type IDs is a transitional solution that will help us experiment with GPU implementations with minimal changes to the existing HiOp code until we understand better how to refactor the API to support hardware accelerator code in a long term. By using matrix/vector type IDs and dynamic casts we can enforce the same matrix compatibility requirements without major refactoring of the API and leaving existing CPU code largely intact.

from hiop.

a transitional solution...

I only worry that any transitional solution will create too much inertia and we'll never actually solve the design challenge. Other than that, LGTM!

from hiop.

Per our discussion, refactoring interfaces would be a much better long-term solution, and per @ashermancinelli's assessment, it wouldn't take too much effort. We could think of inheritance structure like this:

• hiopMatrix - pure virtual base matrix class
  • hiopMatrixDense - virtual, base class for all dense matrix implementations
    • hiopMatrixDenseRowMajor - current implementation of hiopMatrixDense renamed
    • hiopMatrixDenseRowMajorRaja - portable dense matrix implementation using RAJA
  • hiopMatrixSparse - virtual base class for all sparse matrix implementations
    • hiopMatrixSparseTriplet triplet implementation of the sparse matrix
    • hiopMatrixSparseTripletRaja - portable triplet implementation using RAJA
    • hiopMatrixSparseCSR - if ever needed ...

from hiop.

I am a big fan of the naming & inheritance scheme you've laid out. I was in the process of writing up something just like it 😄

In my opinion, we should make this the highest priority so we can build on top of this internally with our additional unit/integration tests. I will make an internal MR and expedite this work unless @cnpetra disagrees.

from hiop.

looking quite good, will take the final look when the PR comes in here.

from hiop.