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

Fix major bug in v0.3.1: negative counts #347

Merged
merged 17 commits into from
Apr 11, 2024
Merged

Conversation

sjfleming
Copy link
Member

@sjfleming sjfleming commented Apr 5, 2024

Closes #306
Closes #348

Something about the update from v0.3.0 to v0.3.1 induced a bug (I'm not yet sure of the scope, but potentially affecting all datasets) where negative counts could be produced in the output. It is assumed that these outputs are totally compromised. v0.3.1 has already been redacted, but the master branch is still compromised.

I think the bug probably lies in the updated posterior generation code, which was the target of memory-saving updates in v0.3.1.

Expand tests covering that code.

  • unit tests for posterior generation
  • integration tests ensuring no negative count matrix entries in the output (not explicitly tested currently)

@sjfleming sjfleming added the bug Something isn't working label Apr 5, 2024
@sjfleming sjfleming self-assigned this Apr 5, 2024
@sjfleming
Copy link
Member Author

After running simulated data through v0.3.0 and the master branch, I see that the posterior generated in each case is identical.

So despite what I initially thought, it seems like the bug must have been introduced in estimation.py, which is quite well-covered with tests...

(It is not a CPU / GPU issue, as a run without --cuda basically agrees with a GPU run.)

@sjfleming
Copy link
Member Author

sjfleming commented Apr 5, 2024

It came down to a small typo in an integer datatype. I had put np.uint8 in estimation.py line 241 here, but np.uint8 is not large enough (it needs to accommodate genes). This was probably causing integer overflows that totally messed up the output count matrix.

Changing to np.uint32 seems to fix the problem.

Before
image
And there are negative count matrix entries in the output.

After
image
And there are no longer negative count matrix entries in the output.

@sjfleming sjfleming marked this pull request as ready for review April 9, 2024 15:28
@sjfleming sjfleming requested a review from mbabadi April 9, 2024 15:28
Copy link
Collaborator

@mbabadi mbabadi left a comment

Choose a reason for hiding this comment

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

Nice job expanding the tests! Just a small comment to address. Also, while you're at it, you may want to search the rest of the codebase for uint8 and other explicit dtypes and refactor accordingly.

@@ -92,11 +93,12 @@ def test_mean_massive_m(log_prob_coo):
new_shape = (coo.shape[0] + greater_than_max_int32, coo.shape[1])
new_coo = sp.coo_matrix((coo.data, (new_row, coo.col)),
shape=new_shape)
print(new_coo)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either remove the print statement, or convert it to something more informative-- e.g. old matrix shape = ..., new matrix shape = ...

@sjfleming
Copy link
Member Author

Alright @mbabadi , what do you think now? I added a warning to the README, and I added a command-line argument to enable people to re-run their old checkpointed v0.3.1 runs to produce a valid output.

Copy link
Collaborator

@mbabadi mbabadi 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!

@sjfleming sjfleming merged commit 97c1d40 into dev Apr 11, 2024
4 checks passed
@sjfleming sjfleming deleted the sf-negative-count-bug-fix branch April 11, 2024 02:39
sjfleming added a commit that referenced this pull request Apr 11, 2024
* Fix major bug in v0.3.1: negative counts (#347)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants