Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Util unit tests #261

Merged
merged 8 commits into from
Mar 16, 2023
Merged

Util unit tests #261

merged 8 commits into from
Mar 16, 2023

Conversation

ebezzi
Copy link
Member

@ebezzi ebezzi commented Mar 15, 2023

Unit tests for:

  • is_positive_integral
  • array_chunker
  • uricat

@ebezzi ebezzi requested a review from bkmartinjr March 15, 2023 18:09
from tools.cell_census_builder.util import array_chunker, is_positive_integral, uricat


def test_is_positive_integral() -> None:
Copy link
Member Author

Choose a reason for hiding this comment

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

To be pedantic, this method should be called is_nonnegative_integral since 0 is an accepted value. If that looks better I can make the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to improve it!

Copy link
Contributor

Choose a reason for hiding this comment

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

If you do this, please also add a test of empty array, e.g.,

assert not is_nonnegative_integral(np.array([], dtype=np.float32))

and equivalent for sparse.

@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #261 (41630bf) into main (0bbe5ea) will increase coverage by 1.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
+ Coverage   92.22%   93.52%   +1.29%     
==========================================
  Files          33       34       +1     
  Lines        1943     2023      +80     
==========================================
+ Hits         1792     1892     +100     
+ Misses        151      131      -20     
Flag Coverage Δ
unittests 93.52% <100.00%> (+1.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tools/cell_census_builder/experiment_builder.py 94.60% <100.00%> (ø)
tools/cell_census_builder/tests/test_util.py 100.00% <100.00%> (ø)
tools/cell_census_builder/util.py 90.27% <100.00%> (+27.26%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more



def test_is_positive_integral() -> None:
X = np.array([1, 2, 3, 4])
Copy link
Contributor

Choose a reason for hiding this comment

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

these result in an array of dtype np.int64. The function only accepts floats (both float32 and float64), although it does not raise an error if the input violates the type signature.

These tests should explicitly create arrays of dtype float32 or float64. Prefer float32 for most tests, as that is what we typically receive.

assert chunked[4].nnz == 19

# Other formats are rejected by the method
X = triu(np.ones(100).reshape(10, 10)).tocoo()
Copy link
Contributor

Choose a reason for hiding this comment

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

recommend testing this by passing something other than COO (eg., LIL or BSR), as it is fairly likely we will add COO support the the array_chunker at some point. Just change the final call from tocoo() to tolil() (or anything other than csr/csc/coo)

Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

couple of change requests. hit me up for a second review when ready.

@ebezzi ebezzi requested a review from bkmartinjr March 15, 2023 20:32
X = csr_matrix([[1, 2, 3], [4, 5, 6]])
assert is_nonnegative_integral(X)

X = csr_matrix([[-1, 2, 3], [4, 5, 6]])
Copy link
Contributor

Choose a reason for hiding this comment

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

these are also int64 - same issue as with ndarray, they need a float dtype to be specified.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies, for some reason I thought scipy.sparse created float matrices by default. Fixing it.

Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

one more dtype issue on the is_nonnegative_integral test.

I'm marking as approved, on the assumption you will add those explicit dtypes....

@ebezzi ebezzi merged commit 22092be into main Mar 16, 2023
@ebezzi ebezzi deleted the ebezzi/util-unit-tests branch March 16, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants