bkochuna / ners570f23-SpMV

1 stars 0 forks source link

Add tests for the ELL matvec method #33

Open KyleVaughn opened 11 months ago

KyleVaughn commented 11 months ago

Description:

Build a test to check the correctness of the ELL matvec kernel. This is a unit test and would not add to any functionality in our library. However, it is a critical component since it validates the ELL matvec kernel.

Tasks:

  1. Read the vecin and vecout values.
  2. Input vecin(x) into ELL matvec kernel; compare its output with vecout.
  3. If the L\inf error is within 1e-3 implies the test has passed
  4. Check how the matvec kernel responds to invalid inputs(for example: empty input vector)

Definition of done:

The tests should be executed when ctest command is passed. If the ELL kernel is correctly implemented, the test should return "All tests passed". If not, then it must exit the code.

Lazy-Beee commented 11 months ago

I would suggest using the function interface virtual fp_type* matvec(fp_type* vecin) and I'll add this to the SparseMatrix class.


Edit: change the api to avoid memory leak void matvec(fp_type* vecin, fp_type* vecout)

KyleVaughn commented 11 months ago

Review by @maxzog

bkochuna commented 11 months ago

Why go through the trouble of creating a dependency in your library for openblas just for testing purposes? Would it be easier to just work with a fixed 100x100 matrix or something small where you can compute the answer by hand or in matlab or something?

What would be the benefit of relying on a random vector every time the test is run?

On Mon, Oct 16, 2023 at 7:44 PM prathameshnakhate @.***> wrote:

Description:

Build functionality to test the correctness of the ELL matvec kernel. Tasks:

  1. Generate a random vecin vector, x
  2. Perform the matric multiplication y=Ax using an efficient and trusted library like OpenBLAS.
  3. Input x into ELL matvec kernel; compare it's output with vecout generated by OpenBLAS.
  4. If the L\inf error is within 1e-4 implies the test has passed

Definition of done:

The tests should be executed when ctest command is passed. If the ELL kernel is correctly implemented, the test should return "All tests passed". If not, then it must exit the code.

— Reply to this email directly, view it on GitHub https://github.com/bkochuna/ners570f23-SpMV/issues/33#issuecomment-1765433883, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHRN4NXL6XGNVEBULWVYNDX7XBGDAVCNFSM6AAAAAA56SL526VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRVGQZTGOBYGM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

maxzog commented 11 months ago

Review: To echo what Prof. Kochunas said, it seems like it'd be better to test the method with a result that you would know exactly as opposed to generating random inputs. It'd be much easier to diagnose any problems with the method if you know what the output should be a priori. Furthermore, based on the discussions we have had in class, if you were to go this route would you not want to test with a more "stable" implementation than OpenBLAS (e.g. BLAS).

Overall I think a little more detail would be nice in the description and a less random testing method would be ideal here.

prathameshnakhate commented 11 months ago

Thanks @bkochuna and @maxzog for the feedback. I was thinking that using a random vector would bring in more robustness, but as you both pointed out, it would become inconvenient due to dependency on OpenBLAS.

We can have a fixed vecin and vecout as we had for HW2. The vecin should be chosen such that it has no zero entries. This will ensure that matvec works for every row/col.

I have updated my tasks accordingly. I have added one extra point to the tasks. Also added a little more detail to the description.