-
Notifications
You must be signed in to change notification settings - Fork 282
Fix support for mathematical types #8324
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
base: develop
Are you sure you want to change the base?
Fix support for mathematical types #8324
Conversation
34b8cbe
to
e5a11d2
Compare
e5a11d2
to
19a8310
Compare
cebcc43
to
9cd88b5
Compare
667b5f5
to
5a6f0b8
Compare
We were missing front-end support for reals and did not consistently support all of integers, naturals, rationals, reals but instead would only handle varying subsets in each place.
5a6f0b8
to
9d13355
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8324 +/- ##
===========================================
+ Coverage 80.43% 80.48% +0.05%
===========================================
Files 1695 1697 +2
Lines 208285 208385 +100
Branches 73 73
===========================================
+ Hits 167524 167726 +202
+ Misses 40761 40659 -102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
class realt | ||
{ | ||
protected: | ||
mp_integer integral, fractional; |
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.
Representing real numbers like floating-point numbers will at the very least irritate. Please add a comment why this is the right choice.
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.
Hmm, which approach would you have chosen instead?
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.
E.g., you could have represented the algebraic numbers via the root of a polynomial; I'd be curious what the various SMT solvers output as models.
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.
z3 and cvc5 seem to output Reals as fractions, unless the denominator is 1, when they just output a singular decimal.
Whilst they use decimals, I've not managed to make it produce a number that isn't an integer as the nominator or denominator, or as a singular decimal.
(set-logic ALL)
(declare-fun x () Real)
(declare-fun y () Real)
(declare-fun z () Real)
(assert (= y (/ 100 30)))
(assert (= x 0.111111))
(assert (= z 7))
(check-sat)
(get-model)
z3 says:
(
(define-fun z () Real
7.0)
(define-fun y () Real
(/ 10.0 3.0))
(define-fun x () Real
(/ 111111.0 1000000.0))
)
cvc5 says
(
(define-fun x () Real (/ 111111 1000000))
(define-fun y () Real (/ 10 3))
(define-fun z () Real 7.0)
)
For irrational numbers, they output the root of a polynomial.
(set-logic ALL)
(declare-fun y () Real)
(assert (= (* y y) 2 ))
(check-sat)
(get-model)
z3 says
(
(define-fun y () Real
(root-obj (+ (^ x 2) (- 2)) 1))
)
cvc5 segfaults...
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 suspect we'll eventually want the algebraic numbers.
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'll try to address this (even when I'd argue that the current PR already addresses a number of CBMC invariant failures that the included regression tests cause when attempted without the code changes).
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.
No need to implement this right now; my suggestion is to add a comment, then merge this PR.
We were missing front-end support for reals and did not consistently support all of integers, naturals, rationals, reals but instead would only handle varying subsets in each place.