#18220 closed defect (fixed)
disallow non exact fields for the 'field' backend
Reported by: | vdelecroix | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.4 |
Component: | geometry | Keywords: | bug, days84 |
Cc: | chapoton, jipilab, mkoeppe | Merged in: | |
Authors: | Vincent Delecroix | Reviewers: | Jean-Philippe Labbé |
Report Upstream: | N/A | Work issues: | |
Branch: | c28fcf0 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
The 'field' backend does not properly support non exact fields
sage: omega = 2*RR.pi() / 5 sage: verts = [((i*omega).sin(), (i*omega).cos()) for i in range(5)] sage: verts sage: Polyhedron(vertices=verts, base_ring=RR) Traceback (most recent call last): ... AssertionError:
or
sage: Q = polytopes.icosahedron() sage: Q = polytopes.icosahedron(); Q A 3-dimensional polyhedron in (Number Field in sqrt5 with defining polynomial x^2 - 5)^3 defined as the convex hull of 12 vertices sage: Q_RR = Polyhedron(vertices = [n(v.vector(),digits=10) for v in Q.vertices()]) sage: Q_RR A 3-dimensional polyhedron in (Real Field with 37 bits of precision)^3 defined as the convex hull of 7 vertices sage: Q_RR = Polyhedron(vertices = [n(v.vector(),digits=30) for v in Q.vertices()]) sage: Q_RR A 3-dimensional polyhedron in (Real Field with 103 bits of precision)^3 defined as the convex hull of 1 vertex and 3 rays
We simply raise an error if somebody want to try this.
Change History (22)
comment:1 Changed 6 years ago by
- Keywords bug added
comment:2 Changed 5 years ago by
comment:3 Changed 5 years ago by
- Cc chapoton added
- Milestone changed from sage-6.7 to sage-7.4
comment:4 Changed 4 years ago by
- Cc jipilab added
- Description modified (diff)
comment:5 Changed 4 years ago by
- Branch set to u/vdelecroix/18220
- Commit set to c28fcf05115ba98f832e74a94eb518bb53117d2a
- Description modified (diff)
- Status changed from new to needs_review
- Summary changed from Bug when creating a polyhedron with coefficients in RR to disallow non exact fields for the 'field' backend
New commits:
c28fcf0 | 18220: raise an error for non exact fields
|
comment:6 Changed 4 years ago by
- Keywords days84 added
comment:7 Changed 4 years ago by
- Cc mkoeppe added
comment:8 Changed 4 years ago by
The tests pass on my machine with 7.6.beta6. The documentation looks fine as well. No idea what the quasar bot is up to.
The I believe it is a good idea to raise an error for such a behaviour.
Matthias, any thoughts? Does that look good for you?
comment:9 Changed 4 years ago by
I agree with disabling it for floating point, but I'm not sure about disabling it for the SR as well. Maybe there are some people who want to work with polytopes with transcendental coordinates?
comment:10 Changed 4 years ago by
I would like to make computations with trascendental coordinates. However, the symbolic ring is not the proper way to do it. I am very much against special casing SR anywhere. SR has its own logic that does not fit at all with the Sage logic of parents and coercion. Moreover, comparison is completely broken
sage: bool(pi == 245850922/78256779) True
comment:11 Changed 4 years ago by
- Reviewers set to Jean-Philippe Labbé
- Status changed from needs_review to positive_review
Then, let's restrict the usage of the symbolic ring with polyhedron.
If it ever happens that there is a use case of the symbolic ring, we may consider adapting it properly then.
comment:12 Changed 4 years ago by
- Status changed from positive_review to needs_info
WAIT!!! Are you proposing to drop support for RR/RDF and other floating points?! With all the bugs it is still useful to be able to operate with them, if you think it is too dangerous, how about throwing a warning?
comment:13 Changed 4 years ago by
As mentioned in the title description, it only concerns the "field" backend that is very much broken for non exact fields. Other backends still support RDF
as it used to be.
comment:14 follow-up: ↓ 15 Changed 4 years ago by
- Status changed from needs_info to positive_review
OK, sorry for the false alarm (it was an end of a tough day yesterday ;-)). I guess I got confused by the strange name for a backend as well...
comment:15 in reply to: ↑ 14 ; follow-up: ↓ 16 Changed 4 years ago by
OK, sorry for the false alarm (it was an end of a tough day yesterday ;-)). I guess I got confused by the strange name for a backend as well...
No worries!
It is true that the name 'field' is confusing...
comment:16 in reply to: ↑ 15 ; follow-up: ↓ 17 Changed 4 years ago by
Replying to jipilab:
OK, sorry for the false alarm (it was an end of a tough day yesterday ;-)). I guess I got confused by the strange name for a backend as well...
No worries!
It is true that the name 'field' is confusing...
What about changing it to 'generic'?
comment:17 in reply to: ↑ 16 ; follow-up: ↓ 18 Changed 4 years ago by
Replying to vdelecroix:
Replying to jipilab:
OK, sorry for the false alarm (it was an end of a tough day yesterday ;-)). I guess I got confused by the strange name for a backend as well...
No worries!
It is true that the name 'field' is confusing...
What about changing it to 'generic'?
or 'generic_exact', or 'sage_toy', or ...
comment:18 in reply to: ↑ 17 ; follow-up: ↓ 19 Changed 4 years ago by
Replying to vdelecroix:
Replying to vdelecroix:
Replying to jipilab:
OK, sorry for the false alarm (it was an end of a tough day yesterday ;-)). I guess I got confused by the strange name for a backend as well...
No worries!
It is true that the name 'field' is confusing...
What about changing it to 'generic'?
or 'generic_exact', or 'sage_toy', or ...
In this setup I would not call the backend generic, because it does not provide the principal reason to use it: irrational exact values.
What about irrational_exact
?
comment:19 in reply to: ↑ 18 ; follow-up: ↓ 20 Changed 4 years ago by
Replying to jipilab:
Replying to vdelecroix:
Replying to vdelecroix:
Replying to jipilab:
OK, sorry for the false alarm (it was an end of a tough day yesterday ;-)). I guess I got confused by the strange name for a backend as well...
No worries!
It is true that the name 'field' is confusing...
What about changing it to 'generic'?
or 'generic_exact', or 'sage_toy', or ...
In this setup I would not call the backend generic, because it does not provide the principal reason to use it: irrational exact values.
What about
irrational_exact
?
It is even more confusing since this backend can deal with rationals... generic_exact
?
comment:20 in reply to: ↑ 19 Changed 4 years ago by
Replying to vdelecroix:
Replying to jipilab:
Replying to vdelecroix:
Replying to vdelecroix:
Replying to jipilab:
OK, sorry for the false alarm (it was an end of a tough day yesterday ;-)). I guess I got confused by the strange name for a backend as well...
No worries!
It is true that the name 'field' is confusing...
What about changing it to 'generic'?
or 'generic_exact', or 'sage_toy', or ...
In this setup I would not call the backend generic, because it does not provide the principal reason to use it: irrational exact values.
What about
irrational_exact
?It is even more confusing since this backend can deal with rationals...
generic_exact
?
Right... hmm.
+1 for generic_exact
.
;-)
comment:21 Changed 4 years ago by
- Branch changed from u/vdelecroix/18220 to c28fcf05115ba98f832e74a94eb518bb53117d2a
- Resolution set to fixed
- Status changed from positive_review to closed
comment:22 Changed 4 years ago by
- Commit c28fcf05115ba98f832e74a94eb518bb53117d2a deleted
I just saw that in the documentation of version 7.6.rc0, the doc file of the backend field
is called "the python backend".
Maybe we should create a ticket about renaming the backend.
Another bug for RDF polyhedra: #21270