Bugfixes for generating dense sketching operators

[kaiwenhe7]

This PR resolves the bug from #57 in a way that supports the functionality mentioned in #53.

[rileyjmurray]

Thanks for re-opening the PR! My first comment concerns the tests. Please remove unnecessary print statements and please enable tests that are currently commented-out.

[rileyjmurray]

@kaiwenhe7 you recently left a comment on PR #57 mentioning that you've tested matrices whose dimensions are prime numbers. Please push those test changes to this branch so that we can inspect them.

[rileyjmurray]

Thanks for that change, @kaiwenhe7. Please also address my comments from earlier:

https://github.com/BallisticLA/RandBLAS/pull/62#issuecomment-1589947904

[TeachRaccooon]

After uncommenting the test cases, seems like we still have an issue.

[kaiwenhe7]

@rileyjmurray @TeachRaccooon I am currently working to try and resolve the uncommented tests. Is it okay if I rewrite the concatenation tests just using fill_dense_submat_impl? I don't fully grasp all of the moving parts of fill_dense_submat. I am currently stuck on the debugging, and it may be good for me to simplify it just using fill_dense_submat_impl.

[TeachRaccooon]

@rileyjmurray @TeachRaccooon I am currently working to try and resolve the uncommented tests. Is it okay if I rewrite the concatenation tests just using fill_dense_submat_impl? I don't fully grasp all of the moving parts of fill_dense_submat. I am currently stuck on the debugging, and it may be good for me to simplify it just using fill_dense_submat_impl.

Sure, whatever works!

[TeachRaccooon]

@kaiwenhe7 any updates?

[kaiwenhe7]

@TeachRaccooon I'm struggling with some things regarding rounding when we're portioning the matrix. I have it working for when the dimensions are even. When we we're splitting the matrix using n_cols / 2, I may be messing up some rounding stuff.

[kaiwenhe7]

@TeachRaccooon There is something interesting from doing more testing. I copied the test_identity function to a separate c++ file and when I ran the exact same test, it seems to not have any errors. Here are some screen shots. If any entry of A and B are not equal the values would be printed out, but when running the file, nothing is printed, which means that each entry should be bitwise equal. On the other hand, the exact same function fails the test in the testing suite.

[kaiwenhe7]

@rileyjmurray I have the truncated version of the submatrix generation implemented along with the exact same tests from the previous implementation. One last choice that needs to be made is the state of the counter than needs to be returned at the end. If we decide to return the state corresponding to the last element of the submatrix, if we want to add rows to the matrix, the user would need to advance to the next counter and then pass it in. Alternatively, we could ourselves by advancing the counter an additional time before returning the state.

[rileyjmurray]

Excellent! I'd like for us to advance the counter one additional time automatically, rather than leave that to the user.

[rileyjmurray]

(Sorry for accidentally closing the PR for a second there.)

[kaiwenhe7]

@rileyjmurray I have the correct state returned and I modified test_identity for the sub matrix concatenation to use the truncated implementation. In the meantime, I will work on the method of returning the correct state that avoids the risk of overflow from the ptr.

[rileyjmurray]

Woohoo!

[kaiwenhe7]

@rileyjmurray Should I delete the old implementations of the submatrix generation?

[rileyjmurray]

@kaiwenhe7 if the new implementation is tested at least as thoroughly as the old implementation, then yes.

[kaiwenhe7]

@rileyjmurray I made all of the changes you requested in our last meeting. In regards to the counter stuff, maybe that can be done in a separate pull request? Let me know what you think of the current changes.

[rileyjmurray]

@kaiwenhe7 I left a comment about a test that's been commented out. Besides that, I agree that we can leave the delicate counter incrementing to the next PR.