-
Notifications
You must be signed in to change notification settings - Fork 9
Adding support for Reals
#50
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
Conversation
This commit adds support for declaring constants of type `Real`, adds the division function for reals `rdiv` and adds support for parsing real valued solutions to f32 and f64.
|
I'll do a more detailed review tonight, but I think that only supporting |
elliottt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking good, thank you for fixing this!
Would you mind adding the test you wrote in a way that we can check it in CI?
elliottt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test you added looks good, thank you! An alternative to fixing the assertion messages would be to use assert_eq!; the only thing I'm really worried about is making sure that the test failures are meaningful.
tests/real_numbers.rs
Outdated
| let solution = ctx.get_value(vec![x]).unwrap(); | ||
| let sol = ctx.get_f64(solution[0].1).unwrap(); | ||
| // Z3 returns `2.0` | ||
| assert!(sol == 2.0, "Expected solution to be 2.5, got {}", sol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assert!(sol == 2.0, "Expected solution to be 2.5, got {}", sol); | |
| assert!(sol == 2.0, "Expected solution to be 2.0, got {}", sol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that you caught that, I never properly updated the messages after using them for debugging.
Since I already used assert_eq! for checking the response, I changed all assertions assert_eq! in 4390749
tests/real_numbers.rs
Outdated
| let solution = ctx.get_value(vec![y]).unwrap(); | ||
| let sol = ctx.get_f64(solution[0].1).unwrap(); | ||
| // Z3 returns `- 2.0` | ||
| assert!(sol == -2.0, "Expected solution to be 2.5, got {}", sol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assert!(sol == -2.0, "Expected solution to be 2.5, got {}", sol); | |
| assert!(sol == -2.0, "Expected solution to be -2.0, got {}", sol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that you caught that, I never properly updated the messages after using them for debugging.
Since I already used assert_eq! for checking the response, I changed all assertions assert_eq! in 4390749
elliottt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This PR would add support for
Realvalues. In more detail:real, with the helper methodreal_sortcon_to_realfor convertingIntvalues to realsrdivwhich corresponds to division for reals (i.e./)On Parsing
RealSolutionsAs it turns out, parsing the satisfying assignment for solutions to real values is not as straightforward. In many cases, the SMT solver will represent a solution as a fraction, e.g. if it assigns the value
2.5to a variable, when asked for the solution, it will return it as the expression/ 5.0 2.0.For my project, I am fine with the loss of precision when evaluating this expression to an
f32orf64. Theget_f32andget_f64methods in this PR will evaluate this expression and return the result. However, if this is not desired, the next section discusses possible alternatives.I have written a small test that should check all possible variations of solutions that can be returned by the SMT solver:
Possible Alternatives
If the loss of precision that occurs when parsing the solution is not desired, I see two alternatives:
get_numeratorandget_denominatorLet me know your thoughts!