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

Herbie doesn't know that cos(x) <= 1 #840

Closed
oscardssmith opened this issue Jun 5, 2024 · 2 comments · Fixed by #867
Closed

Herbie doesn't know that cos(x) <= 1 #840

oscardssmith opened this issue Jun 5, 2024 · 2 comments · Fixed by #867

Comments

@oscardssmith
Copy link

When asking herbie to optimize cos(y)-x (with ranges 0 to 1e3 for both variables), it suggest the approximation:

if cos(y)≤1:
    cos(y)
else:
     1−x​​

asides from the correctness issues mentioned in #742, it does seem that herbie should realize that the first branch will always be taken. Also the cost model is off since it seems to think that this program requires computing cos(y) twice.

@pavpanchekha
Copy link
Contributor

This is pretty funny and I'm not sure what causes it. Thinking through how the if statements gets there it seems like this shouldn't occur.

@pavpanchekha
Copy link
Contributor

Ok—I dug in and I think I know why this happens and how to fix it. Basically:

  • These if statements have three parts: the expression on the left hand side, the value on the right hand side, and then the stuff that goes in the then and else cases
  • Here the issue is with the right hand side, which is always a single number

To compute this single number, we first bound it between two values we sampled. We refer to the value by an index in the point list. Sometimes two values are the same, and then we can't insert a split in between.

We're not consistent with what the index refers to: the left-hand value or the right-hand value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants