Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#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) Commit:
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

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 5 years ago by vdelecroix

  • Keywords bug added

comment:2 Changed 4 years ago by mkoeppe

Another bug for RDF polyhedra: #21270

Last edited 4 years ago by mkoeppe (previous) (diff)

comment:3 Changed 4 years ago by mkoeppe

  • Cc chapoton added
  • Milestone changed from sage-6.7 to sage-7.4

comment:4 Changed 3 years ago by jipilab

  • Cc jipilab added
  • Description modified (diff)

comment:5 Changed 3 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • 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:

c28fcf018220: raise an error for non exact fields

comment:6 Changed 3 years ago by vdelecroix

  • Keywords days84 added

comment:7 Changed 3 years ago by jipilab

  • Cc mkoeppe added

comment:8 Changed 3 years ago by jipilab

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 3 years ago by mkoeppe

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 3 years ago by vdelecroix

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 3 years ago by jipilab

  • 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 3 years ago by novoselt

  • 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 3 years ago by vdelecroix

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: Changed 3 years ago by novoselt

  • 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: Changed 3 years ago by 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...

comment:16 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by 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'?

comment:17 in reply to: ↑ 16 ; follow-up: Changed 3 years ago by 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 ...

comment:18 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by 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 ?

comment:19 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by 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?

comment:20 in reply to: ↑ 19 Changed 3 years ago by jipilab

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 3 years ago by vbraun

  • Branch changed from u/vdelecroix/18220 to c28fcf05115ba98f832e74a94eb518bb53117d2a
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 Changed 3 years ago by jipilab

  • 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.

Note: See TracTickets for help on using tickets.