bkochuna / ners570f23-SpMV

1 stars 0 forks source link

JDS Branch #70

Open rgrawe opened 10 months ago

rgrawe commented 10 months ago

This is the branch for JDS. Review Issues 25 and 20 here.

nzamb222 commented 10 months ago

Review of Add JDS Matrix Class issue #20 @shujhan . Default constructor look good, nice job. Your destructor needs to clear the memory for the colIdx , val , iterptr, and row . Consider adding delete[] and setting it to a null pointer in the destructor call in the .cpp file. The overloaded constructor is declared correctly, but I am unsure if you need to assign values to all the variables? I believe that is the job of the assembleStorage(). You can keep it like this however, just might be duplicated.

rgrawe commented 10 months ago

Review of Add JDS Matrix Class issue #20 @shujhan . Default constructor look good, nice job. Your destructor needs to clear the memory for the colIdx , val , iterptr, and row . Consider adding delete[] and setting it to a null pointer in the destructor call in the .cpp file. The overloaded constructor is declared correctly, but I am unsure if you need to assign values to all the variables? I believe that is the job of the assembleStorage(). You can keep it like this however, just might be duplicated.

The constructor should only be calling nrows and ncols. This is the format that I've assumed for the JDS constructor test. You can look at the other sparse matrix constructors for an example.

shujhan commented 10 months ago

Thanks for the comments. I have uploaded the revision version.

shujhan commented 10 months ago

@nzamb222 Thanks, changed it.

shujhan commented 10 months ago

Review #25 @rgrawe Looks good but will JDS class have .getFormat() method?

rgrawe commented 10 months ago

@shujhan No idea! I included it because the COO constructor test included it, but I can get rid of it if you think it's unnecessary.