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 integer overflow in RGroupDecomposition strategy GA #5460

Merged
merged 2 commits into from
Aug 3, 2022

Conversation

bp-kelley
Copy link
Contributor

This fix an issue where the number of permutations would overflow in the GA and be set to 0.

This caused the code to fallback to the Exhaustive search which basically hangs at this point.

@bp-kelley bp-kelley requested a review from ptosco July 26, 2022 16:54
@bp-kelley
Copy link
Contributor Author

@jones-gareth please take a look at this fix. We've been having some hanging issues (well, effectively hanging issues) that this seems to resolve.

@bp-kelley bp-kelley changed the title Fix integer overflow in numPermutations Fix integer overflow in RGroupDecomposition strategy GA Jul 26, 2022
@bp-kelley
Copy link
Contributor Author

I don't have a test case here as (a) it was large and (b) I noticed it on in house data

@bp-kelley
Copy link
Contributor Author

@greglandrum code is updated with std::min

@bp-kelley bp-kelley added the bug label Jul 29, 2022
@bp-kelley
Copy link
Contributor Author

@greglandrum any chance on a merge for this one?

Copy link
Member

@greglandrum greglandrum left a comment

Choose a reason for hiding this comment

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

LGTM

@greglandrum
Copy link
Member

@greglandrum any chance on a merge for this one?

Sure. I was just waiting to see if @jones-gareth had any comments.

@greglandrum greglandrum merged commit 20673d4 into rdkit:master Aug 3, 2022
@greglandrum greglandrum added this to the 2022_03_5 milestone Aug 3, 2022
greglandrum pushed a commit that referenced this pull request Aug 4, 2022
* Fix integer overflow in numPermutations

* Use Greg's suggestion to simplify code

Co-authored-by: Brian Kelley <bkelley@relaytx.com>
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.

None yet

3 participants