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

BUG: optimize.differential_evolution: fix division by zero error #18392

Merged
merged 1 commit into from
May 1, 2023

Conversation

stepeos
Copy link
Contributor

@stepeos stepeos commented Apr 30, 2023

Reference issue

Closing possible division by zero bug #18388

What does this implement/fix?

Adding the machine epslion after taking the absolute to avoid division by zero.

@stepeos stepeos requested a review from andyfaff as a code owner April 30, 2023 21:05
@j-bowhay j-bowhay changed the title BUG: #18388 division by zero BUG: optimize.differential_evolution: fix division by zero error Apr 30, 2023
@j-bowhay j-bowhay added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.optimize labels Apr 30, 2023
@mdhaber
Copy link
Contributor

mdhaber commented Apr 30, 2023

Thanks @stepeos @ChVeen.
Is there a test problem that would experience the divide by zero without this patch and is fixed by this patch? It would be good to add that as a unit test in scipy/optimize/tests/test__differential_evolution.py following the style of test_gh_4511_regression, which identifies what the test is for in a comment.
Adding that would make it easy for @andyfaff, the author, to review and merge.
(BTW, in the future, please open PRs from a feature branch rather than your main.)

@andyfaff
Copy link
Contributor

andyfaff commented May 1, 2023

Thanks for this PR.

Yes, it was to prevent division by zero. Having a mean population energy of zero is not unknown, but I doubt the bug would've seriously bitten anyone.

As @mdhaber says, testing for regressions is an important part of the project. However, in this case I'm sufficiently confident that the fix is straightforward to permit a merge without it.

The reason a feature branch is important is that it allows your fork/main to be updated easily without issues, allowing you to work on many independent feature branches without them interfering with each other.

There are also guidelines for how to construct commit messages at https://docs.scipy.org/doc/scipy/dev/contributor/development_workflow.html#writing-the-commit-message.

@andyfaff
Copy link
Contributor

andyfaff commented May 1, 2023

Merging without CI running, it's a simple one-liner and I checked bracket pairing locally.

@andyfaff andyfaff merged commit 92d767c into scipy:main May 1, 2023
5 of 20 checks passed
@ilayn ilayn added this to the 1.11.0 milestone May 1, 2023
@j-bowhay j-bowhay linked an issue May 1, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.optimize
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question about usage of _MACHEPS
5 participants