Skip to content
This repository was archived by the owner on May 15, 2025. It is now read-only.

Fix ITP for problems with flipped sign #72

Merged
merged 1 commit into from
Jul 21, 2023
Merged

Fix ITP for problems with flipped sign #72

merged 1 commit into from
Jul 21, 2023

Conversation

DaniGlez
Copy link
Contributor

@DaniGlez DaniGlez commented Jul 21, 2023

For some problems, there's a potential bug as the current ITP implementation follows the pseudocode in the Oliveira - Takahashi paper, which assumes f(left) < 0 < f(right), and thus it may wrongly contract the interval for problems with the reverse sign configuration (surprisingly enough, it sometimes works with the wrong signs [1]). This PR fixes the issue and adds a basic test for it.

[1] Did not realize that's only the case if one of the edges is the solution already

Copy link
Member

@ChrisRackauckas ChrisRackauckas left a comment

Choose a reason for hiding this comment

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

Thanks! And that test will be super helpful as well.

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #72 (fe74b25) into main (8717ca6) will increase coverage by 0.70%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
+ Coverage   91.78%   92.48%   +0.70%     
==========================================
  Files          21       21              
  Lines        1010     1011       +1     
==========================================
+ Hits          927      935       +8     
+ Misses         83       76       -7     
Impacted Files Coverage Δ
src/itp.jl 93.22% <100.00%> (+3.56%) ⬆️

... and 5 files with indirect coverage changes

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

@yash2798
Copy link
Member

Thanks @DaniGlez for the fix :)

@ChrisRackauckas ChrisRackauckas merged commit 59c9f4c into SciML:main Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants