Opened 5 years ago

Last modified 4 years ago

#22605 closed defect

Better error handling of the polyhedron constructor for non-embedded NumberField and floats. — at Version 33

Reported by: jipilab Owned by:
Priority: major Milestone: sage-8.1
Component: geometry Keywords: polyhedron, base ring
Cc: moritz, mkoeppe, vdelecroix, novoselt, tmonteil, tscrim Merged in:
Authors: Jean-Philippe Labbé, Vincent Delecroix Reviewers:
Report Upstream: N/A Work issues:
Branch: u/jipilab/22605 (Commits, GitHub, GitLab) Commit: fc5089dcb4d33ca2e7b0595a57d8cbc4dccdd733
Dependencies: Stopgaps:

Status badges

Description (last modified by jipilab)

These error messages are not really informative of what the problem really is:

sage: K = NumberField(x^2-2,'s')
sage: s = K.0
sage: L = NumberField(x^3-2,'t')
sage: t = L.0
sage: P = Polyhedron(vertices = [[0,s],[t,0]])
Traceback (most recent call last):
...
AttributeError: 'Objects_with_category' object has no attribute 'is_exact'

similar problem with floats:

sage: f = float(1.1)
sage: f
1.1
sage: Polyhedron(vertices=[[f]])
Traceback (most recent call last):
...
AttributeError: type object 'float' has no attribute 'is_exact'

The constructor could try to detect the problem and return an informative message.

Once this is fixed, ticket #22552 is a follow-up ticket that should be fixed easily from the present resolution.

Change History (33)

comment:1 Changed 5 years ago by jipilab

  • Description modified (diff)

comment:2 Changed 5 years ago by jipilab

  • Cc novoselt added

comment:3 Changed 5 years ago by vdelecroix

Indeed. Do we want to convert float -> RDF these two are fully compatible?

comment:4 Changed 5 years ago by jipilab

If this:

sage: RDF.has_coerce_map_from(float)
True

is all we need, I guess so... It would do a similar trick as for RR with 53 bits of precision...

comment:5 follow-up: Changed 5 years ago by vdelecroix

It is not: coercion has nothing to do with embedding

sage: Zmod(5).has_coerce_map_from(ZZ)
True
sage: RealField(5).has_coerce_map_from(RealField(256))
True

comment:6 in reply to: ↑ 5 Changed 5 years ago by jipilab

So how to check for the embedding? Simply wrap?

comment:7 Changed 5 years ago by jipilab

  • Branch set to u/jipilab/22605

comment:8 Changed 5 years ago by jipilab

  • Commit set to 8678e1784b9bd7bbce8baa84e8846ab8d96649da

I added a way to catch the float and set the base ring to RDF. Now, I'm wondering how we could handle the algebraic numbers in an efficient way. Indeed, it seems that they are automatically converted while they might not need it.


New commits:

8678e17Made support for floats

comment:9 Changed 4 years ago by git

  • Commit changed from 8678e1784b9bd7bbce8baa84e8846ab8d96649da to 47d9425ef1283708b7a4ce0a7e34a93c2d16b2dc

Branch pushed to git repo; I updated commit sha1. New commits:

1235ae4Merge branch 'u/jipilab/22605' of git://trac.sagemath.org/sage into 22605
47d9425Added handling of not embedded NumberFields and tests

comment:10 Changed 4 years ago by jipilab

  • Status changed from new to needs_review

comment:11 Changed 4 years ago by git

  • Commit changed from 47d9425ef1283708b7a4ce0a7e34a93c2d16b2dc to 9567999c707cc3defd2d416cfadcae6f148937a6

Branch pushed to git repo; I updated commit sha1. New commits:

9567999pep8 and fix a typo

comment:12 Changed 4 years ago by vdelecroix

A polyhedron with one non embedded number field should already gives an error. Could you add an example involving only L?

comment:13 Changed 4 years ago by vdelecroix

Instead of the manual float -> RDF you should use py_scalar_parent from sage.structure.coercion. By the way, where the conversion Python int -> ZZ is happening? This should be done at the same time.

comment:14 Changed 4 years ago by vdelecroix

  • Branch changed from u/jipilab/22605 to u/vdelecroix/22605
  • Commit changed from 9567999c707cc3defd2d416cfadcae6f148937a6 to 97694dd9bda1746e617b8c9d2ce3f9cf0433a841

Much more resaonable code path in my branch. Though the error message could still be elaborated.


New commits:

97694dd22605: simpler constructor for Polyhedron

comment:15 Changed 4 years ago by vdelecroix

BTW, I think that the following would better be handled by Polyhedra (constructor of the parent)

# TODO: find a more robust way of checking that the coefficients are indeed
# real numbers
if base_ring not in Rings() or not RDF.has_coerce_map_from(base_ring):
    raise ValueError("invalid base ring")

comment:16 Changed 4 years ago by jipilab

I was also hesitating about where to put the handling of the error... But since there was not proper base_ring to pass to the parent, I thought it is better to stop there... But then, looking up at the documentation on top of the constructor... there is not much...

I then looked at the output of Polyhedron? and it is there... So I guess it is a good idea to put it in the constructor?

I will have a deeper look at your commit asap. I would be happy to have a cleaner and clearer constructor...

comment:17 follow-up: Changed 4 years ago by jipilab

  • Status changed from needs_review to needs_work

I'm looking at the last commit and at line 488 of the constructor, I got puzzled a bit...

If you give a polyhedron with an H-representation and specified the base_ring=ZZ, then it would convert it automatically to QQ? What if the user is clever and knows that the H-representation that is given really describes a lattice polytope and would like to specify its parent to be Polyhedron_ZZ. So forcing the base_ring should be possible?

I have #22754 in mind as an example.

I'll try to put some time on this, but I will look at the trac only sporadically in the next 2 weeks.

comment:18 follow-up: Changed 4 years ago by mkoeppe

Overall I think the distinction of base_ring=QQ and base_ring=ZZ is completely unnecessary. The constructor could just unconditionally replace the given base ring by its fraction field.

comment:19 in reply to: ↑ 18 Changed 4 years ago by jipilab

Replying to mkoeppe:

Overall I think the distinction of base_ring=QQ and base_ring=ZZ is completely unnecessary. The constructor could just unconditionally replace the given base ring by its fraction field.

But this distinction is used in order to pick the proper parent isn't it? So what about the distinction of the two parent classes _QQ and _ZZ? It is true that a rational polytope has an Ehrhart quasipolynomial which is not necessarily a polynomial... Well, well...

It is true that this distinction makes life more difficult in some cases...

comment:20 follow-up: Changed 4 years ago by mkoeppe

Yes, that's what I mean; there is no need to distinguish the parents for QQ and ZZ.

The method for computing an ehrhart polynomial should also be available for rational polytopes; being a lattice polytope is merely a sufficient condition for getting an ehrhart polynomial. If the user asks for the ehrhart polynomial but the counting function is a quasipolynomial (with a true period), then an exception should be raised by the method.

comment:21 in reply to: ↑ 20 Changed 4 years ago by vdelecroix

Replying to mkoeppe:

Yes, that's what I mean; there is no need to distinguish the parents for QQ and ZZ.

The method for computing an ehrhart polynomial should also be available for rational polytopes; being a lattice polytope is merely a sufficient condition for getting an ehrhart polynomial. If the user asks for the ehrhart polynomial but the counting function is a quasipolynomial (with a true period), then an exception should be raised by the method.

As far as I understand, what you propose is way beyond the scope of this ticket. The simplification I wrote keep the ancient behavior (up to the fact that rationals are now treated as rationals).

comment:22 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by vdelecroix

Replying to jipilab:

I'm looking at the last commit and at line 488 of the constructor, I got puzzled a bit...

If you give a polyhedron with an H-representation and specified the base_ring=ZZ, then it would convert it automatically to QQ? What if the user is clever and knows that the H-representation that is given really describes a lattice polytope and would like to specify its parent to be Polyhedron_ZZ. So forcing the base_ring should be possible?

True! The constructor should not overwrite base_ring if it was set by the user. I will add a commit.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 4 years ago by vdelecroix

Replying to vdelecroix:

Replying to jipilab:

I'm looking at the last commit and at line 488 of the constructor, I got puzzled a bit...

If you give a polyhedron with an H-representation and specified the base_ring=ZZ, then it would convert it automatically to QQ? What if the user is clever and knows that the H-representation that is given really describes a lattice polytope and would like to specify its parent to be Polyhedron_ZZ. So forcing the base_ring should be possible?

True! The constructor should not overwrite base_ring if it was set by the user. I will add a commit.

Where is this happening!? I don't see the code path that you are mentioning...

comment:24 Changed 4 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:25 in reply to: ↑ 23 Changed 4 years ago by jipilab

Replying to vdelecroix:

Replying to vdelecroix:

Replying to jipilab:

I'm looking at the last commit and at line 488 of the constructor, I got puzzled a bit...

If you give a polyhedron with an H-representation and specified the base_ring=ZZ, then it would convert it automatically to QQ? What if the user is clever and knows that the H-representation that is given really describes a lattice polytope and would like to specify its parent to be Polyhedron_ZZ. So forcing the base_ring should be possible?

True! The constructor should not overwrite base_ring if it was set by the user. I will add a commit.

Where is this happening!? I don't see the code path that you are mentioning...

Sorry, got confused in reading the code and running an example in my head. If the user specified a base_ring it won't go where I thought it would because of the if, elif, else clause... So nevermind... my bad!

comment:26 Changed 4 years ago by vdelecroix

  • Authors set to Jean-Philippe Labbé, Vincent Delecroix

comment:27 Changed 4 years ago by jipilab

  • Branch changed from u/vdelecroix/22605 to u/jipilab/22605

comment:28 Changed 4 years ago by jipilab

  • Commit changed from 97694dd9bda1746e617b8c9d2ce3f9cf0433a841 to dc0e5244f046105e39281c6af82f60aeb3a94440

Oops, sorry. Didn't want to change the branch to do the rebase. My bad...

I'll change it back.


New commits:

dc0e524Rebased on Sage-8.0.beta3

comment:29 Changed 4 years ago by git

  • Commit changed from dc0e5244f046105e39281c6af82f60aeb3a94440 to fc5089dcb4d33ca2e7b0595a57d8cbc4dccdd733

Branch pushed to git repo; I updated commit sha1. New commits:

97694dd22605: simpler constructor for Polyhedron
fc5089dMerge branch 'u/vdelecroix/22605' into 22605

comment:30 Changed 4 years ago by jipilab

Tests passed on base.py and constructor.py with Sage-8.0.beta3.

The bots didn't have a look at this yet?!

comment:31 Changed 4 years ago by vdelecroix

The bots are not testing many things see https://github.com/sagemath/sage-patchbot/issues/76

comment:32 Changed 4 years ago by vdelecroix

(I just launched mine manually)

comment:33 Changed 4 years ago by jipilab

  • Description modified (diff)
Note: See TracTickets for help on using tickets.