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 posterior and estimator integer overflow bugs on Windows #259

Merged
merged 10 commits into from
Oct 31, 2023

Conversation

sjfleming
Copy link
Member

The data structure used to store the full posterior is a [m, c] matrix, where m is n_barcodes times n_genes. So m can be absolutely massive.

When m > maximum possible int32:

Including explicit casts to uint64 seems to solve the problem on Windows (pending input on #252 ).

I have included a test which I believe might fail on Windows without this fix, though I do not have a Windows setup to test on yet.

@sjfleming sjfleming added the bug Something isn't working label Aug 25, 2023
@ps2504
Copy link

ps2504 commented Aug 25, 2023

Hello,

I made the two code changes in posterior.py, however, I still get the exact same error. I'm not sure how to run the "test" using the files within the test folder...

Thanks,
Paul

cellbender:remove-background: Computing target noise counts per gene for MCKP estimator
Traceback (most recent call last):
File "C:\Users\SuP1.conda\envs\cellbender\Scripts\cellbender-script.py", line 33, in
sys.exit(load_entry_point('cellbender', 'console_scripts', 'cellbender')())
File "c:\users\sup1\downloads\cellbender-0.3.0\cellbender-0.3.0\cellbender\base_cli.py", line 123, in main
cli_dict[args.tool].run(args)
File "c:\users\sup1\downloads\cellbender-0.3.0\cellbender-0.3.0\cellbender\remove_background\cli.py", line 185, in run
return main(args)
File "c:\users\sup1\downloads\cellbender-0.3.0\cellbender-0.3.0\cellbender\remove_background\cli.py", line 230, in main
posterior = run_remove_background(args)
File "c:\users\sup1\downloads\cellbender-0.3.0\cellbender-0.3.0\cellbender\remove_background\run.py", line 133, in run_remove_background
file_name=file_name,
File "c:\users\sup1\downloads\cellbender-0.3.0\cellbender-0.3.0\cellbender\remove_background\run.py", line 237, in compute_output_denoised_counts_reports_metrics
per_gene=True,
File "c:\users\sup1\downloads\cellbender-0.3.0\cellbender-0.3.0\cellbender\remove_background\posterior.py", line 1579, in compute_mean_target_removal_as_function
device=device,
File "c:\users\sup1\downloads\cellbender-0.3.0\cellbender-0.3.0\cellbender\remove_background\estimation.py", line 150, in estimate_noise
dtype=np.float32)
File "c:\users\sup1\downloads\cellbender-0.3.0\cellbender-0.3.0\cellbender\remove_background\estimation.py", line 77, in _estimation_array_to_csr
dtype=dtype,
File "c:\users\sup1\downloads\cellbender-0.3.0\cellbender-0.3.0\cellbender\remove_background\estimation.py", line 238, in _estimation_array_to_csr
row, col = index_converter.get_ng_indices(m_inds=m)
File "c:\users\sup1\downloads\cellbender-0.3.0\cellbender-0.3.0\cellbender\remove_background\posterior.py", line 1537, in get_ng_indices
raise ValueError(f'Requested m_inds out of range: '
ValueError: Requested m_inds out of range: [-2147483648 -2147483648 -2147483648 ... -2147483648 -2147483648
-2147483648]

@sjfleming
Copy link
Member Author

@ps2504 it looks like you have cloned the github repository to your local machine, so all you have to do to run the relevant tests would be (in the folder where the cellbender code is located)

git checkout sf_posterior_int_overflow
pip install pytest
pytest -k test_save_and_load cellbender/remove_background/tests/test_posterior.py

if tests pass, you'd see an output like this

=================================================== test session starts ===================================================
platform darwin -- Python 3.7.9, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
rootdir: /Users/sfleming/Documents/Github/CellBender
collected 76 items / 72 deselected / 4 selected                                                                           

cellbender/remove_background/tests/test_posterior.py ....                                                           [100%]

============================================ 4 passed, 72 deselected in 0.19s =============================================

But based on what you said about getting the same error, I'd be expecting the test to fail. I will try to add a Windows configuration to the automatic tests that run in github.

@ps2504
Copy link

ps2504 commented Aug 25, 2023

Hello again,

Okay I was able to run the test per your instructions: INTERESTINGLY, the test looks like there's no issues. but I went back and reran the code again but got the same error

(cellbender) C:\Users\SuP1\Downloads\CellBender>pytest -k test_save_and_load cellbender/remove_background/tests/test_posterior.py
=========================== test session starts ============================
platform win32 -- Python 3.7.16, pytest-7.4.0, pluggy-1.2.0
rootdir: C:\Users\SuP1\Downloads\CellBender
plugins: anyio-3.5.0
collected 76 items / 72 deselected / 4 selected

cellbender\remove_background\tests\test_posterior.py .... [100%]

===================== 4 passed, 72 deselected in 0.18s =====================

@ps2504
Copy link

ps2504 commented Aug 25, 2023

The last call back error was in posterior.py, but the one prior is in estimation.py. I have no idea but could that have something to do with it?

Thank you so much for looking into it.

@ps2504
Copy link

ps2504 commented Aug 26, 2023

I pasted the error into chatgpt and this is what it returned... I hope this might be useful?

The error message you provided indicates that there is an issue with the input data, specifically with the indices provided in the m_inds array. It seems like the values in the m_inds array are set to -2147483648, which is the minimum value for a 32-bit signed integer (int32). This value is often used as a sentinel value or a placeholder in various programming contexts.

Here are a few things you can check and consider to diagnose and address this issue:

Index Range: The error message suggests that the values in m_inds are out of range. Check if the indices you're using are within the valid range for the data structure or function you're using. Make sure that the indices are non-negative and within the appropriate range for the specific operation you're performing.

Data Type: Ensure that the data type of the m_inds array is correct. If the data type is incorrectly specified or if there's a type mismatch, it can lead to unexpected behavior or errors. For example, using a data type that is too small to represent the indices can result in overflow or wrapping of values.

Initialization: If the m_inds array is being initialized with default values, such as -2147483648, it might indicate an issue with the way the array is being created or populated. Make sure that the array is being initialized correctly and that the values are being assigned as intended.

Data Integrity: Check the source of the data you're using to populate the m_inds array. It's possible that there's a bug or a data preprocessing issue that's causing incorrect values to be assigned to the array.

Library or Framework Specifics: If you're using a specific library or framework for numerical computations, consult its documentation to understand the expected input formats and data ranges. Different libraries might have different requirements for input data.

Data Exploration: Before passing the data to the function that's causing the error, print out the contents of the m_inds array to see if the values are indeed what you expect them to be. This can help you identify any inconsistencies or unexpected values.

Without more context about the specific code you're working with and the function that's causing the error, it's a bit challenging to provide a more precise solution. However, I hope these general suggestions help you identify and resolve the issue you're facing.

@ps2504
Copy link

ps2504 commented Aug 28, 2023

Hi. Just wanted to update that the most recent changes you made (Make dtype of m explicit in apply_function_dense_chunks) seems to have done the trick.

Thanks a bunch!

@sjfleming
Copy link
Member Author

@ps2504 wonderful! Great news!

@sjfleming sjfleming requested a review from mbabadi August 28, 2023 16:41
@sjfleming sjfleming changed the title Fix posterior integer overflow bug Fix posterior and estimator integer overflow bugs on Windows Aug 28, 2023
@sjfleming sjfleming marked this pull request as ready for review August 28, 2023 16:43
offset_dict = {k + greater_than_max_int32: v for k, v in log_prob_coo['offsets'].items()}

# this is just a shim
converter = IndexConverter(total_n_cells=2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a very cool library for "mock"ing classes and methods for unit tests:
https://docs.python.org/3/library/unittest.mock.html

(just for future reference)

@sjfleming sjfleming merged commit 12b4758 into dev Oct 31, 2023
sjfleming added a commit that referenced this pull request Oct 31, 2023
* Add WDL input to set number of retries. (#247)

* Move hash computation so that it is recomputed on retry, and now-invalid checkpoint is not loaded. (#258)

* Bug fix for WDL using MTX input (#246)

* Memory-efficient posterior generation (#263)

* Fix posterior and estimator integer overflow bugs on Windows (#259)

* Move from setup.py to pyproject.toml (#240)

* Fix bugs with report generation across platforms (#302)

---------

Co-authored-by: kshakir <github@kshakir.org>
Co-authored-by: alecw <alecw@users.noreply.github.com>
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.

3 participants