Skip to content

Unit testing for Sparse matrix class#294

Merged
shakedregev merged 16 commits intodevelopfrom
adham/sparse-tests
Jun 5, 2025
Merged

Unit testing for Sparse matrix class#294
shakedregev merged 16 commits intodevelopfrom
adham/sparse-tests

Conversation

@adhamsi
Copy link
Copy Markdown
Collaborator

@adhamsi adhamsi commented Jun 2, 2025

Description

Implements unit testing for Sparse.cpp.

Closes #280

Proposed changes

Tests implemented:

  • constructor
  • setting data pointers
  • setting values pointer
  • copying values
  • copying values, then attempting to set data/values (must result in error)
  • allocating and destroying memory

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here
to help! This is simply a reminder of what we are going to look for before
merging your code.

  • All tests pass. Code tested on
    • CPU backend
    • CUDA backend
    • HIP backend
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows Re::Solve style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.

Further comments

Comment thread tests/unit/matrix/SparseTests.hpp Outdated
Comment thread tests/unit/matrix/SparseTests.hpp Outdated
Comment thread tests/unit/matrix/SparseTests.hpp Outdated
Copy link
Copy Markdown
Collaborator

@shakedregev shakedregev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are spuriously passing on Frontier (likely due to some pointer comparison that happens to be true). They are failing on CUDA, almost certainly because of the code I pointed out (and other places where it's repeated).

Comment thread tests/unit/matrix/SparseTests.hpp Outdated
Copy link
Copy Markdown
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start but requires some more work. Setting this as a draft.

Comment thread tests/unit/matrix/SparseTests.hpp Outdated
Comment thread tests/unit/matrix/SparseTests.hpp Outdated
Comment thread tests/unit/matrix/SparseTests.hpp Outdated
Comment thread tests/unit/matrix/SparseTests.hpp Outdated
Comment thread tests/unit/matrix/SparseTests.hpp
Comment thread tests/unit/matrix/SparseTests.hpp Outdated
Comment thread tests/unit/matrix/SparseTests.hpp
@pelesh pelesh marked this pull request as draft June 3, 2025 14:50
@pelesh pelesh added the testing label Jun 3, 2025
@pelesh
Copy link
Copy Markdown
Collaborator

pelesh commented Jun 3, 2025

There is a minor warning that needs fixing:

In file included from /.../src/resolve/resolve/tests/unit/matrix/runSparseTests.cpp:8:
/.../src/resolve/resolve/tests/unit/matrix/SparseTests.hpp:186:40: warning: variable 'h_val_data' is uninitialized when used here [-Wuninitialized]
  186 |             mem_.copyArrayDeviceToHost(h_val_data, A->getValues(memory::DEVICE), nnz);
      |                                        ^~~~~~~~~~
/.../src/resolve/resolve/tests/unit/matrix/SparseTests.hpp:182:32: note: initialize the variable 'h_val_data' to silence this warning
  182 |           real_type* h_val_data;
      |                                ^
      |                                 = nullptr

@adhamsi adhamsi force-pushed the adham/sparse-tests branch from 009cbde to ade6772 Compare June 3, 2025 17:32
@adhamsi adhamsi force-pushed the adham/sparse-tests branch from a40e456 to 12f6c7e Compare June 5, 2025 12:58
@adhamsi adhamsi marked this pull request as ready for review June 5, 2025 12:59
@adhamsi adhamsi requested review from pelesh and shakedregev June 5, 2025 12:59
@adhamsi adhamsi marked this pull request as draft June 5, 2025 13:07
@adhamsi adhamsi marked this pull request as ready for review June 5, 2025 17:12
Comment thread tests/unit/matrix/SparseTests.hpp Outdated
Comment thread tests/unit/matrix/SparseTests.hpp
Comment thread tests/unit/matrix/SparseTests.hpp Outdated
@adhamsi adhamsi force-pushed the adham/sparse-tests branch from 8cede43 to bc4d3c4 Compare June 5, 2025 19:36
Copy link
Copy Markdown
Collaborator

@shakedregev shakedregev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and tests pass. Just add tests for rectangular matrices and let's make sure they still pass.

@shakedregev shakedregev requested review from pelesh and shakedregev June 5, 2025 20:40
Copy link
Copy Markdown
Collaborator

@shakedregev shakedregev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass and all comments addressed. Approving

@shakedregev shakedregev merged commit d96f37b into develop Jun 5, 2025
4 checks passed
@shakedregev shakedregev deleted the adham/sparse-tests branch June 5, 2025 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write unit tests for Sparse (matrix) class methods

3 participants