Opened 3 years ago

Closed 2 years ago

#22605 closed defect (fixed)

Better error handling of the polyhedron constructor for non-embedded NumberField and floats.

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, Marcelo Forets Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: af03de8 (Commits) Commit: af03de82ed07686322db70895a0ab34973888981
Dependencies: #23345 Stopgaps:

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 (65)

comment:1 Changed 3 years ago by jipilab

  • Description modified (diff)

comment:2 Changed 3 years ago by jipilab

  • Cc novoselt added

comment:3 Changed 3 years ago by vdelecroix

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

comment:4 Changed 3 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 3 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 3 years ago by jipilab

So how to check for the embedding? Simply wrap?

comment:7 Changed 3 years ago by jipilab

  • Branch set to u/jipilab/22605

comment:8 Changed 3 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 3 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 3 years ago by jipilab

  • Status changed from new to needs_review

comment:11 Changed 3 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 3 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 3 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 3 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 3 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 3 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 3 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 3 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 3 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 3 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 3 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 3 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 3 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 3 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:25 in reply to: ↑ 23 Changed 3 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 2 years ago by vdelecroix

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

comment:27 Changed 2 years ago by jipilab

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

comment:28 Changed 2 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 2 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 2 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 2 years ago by vdelecroix

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

comment:32 Changed 2 years ago by vdelecroix

(I just launched mine manually)

comment:33 Changed 2 years ago by jipilab

  • Description modified (diff)

comment:34 Changed 2 years ago by vdelecroix

.. TESTS: should be TESTS: (no .. )

comment:35 Changed 2 years ago by vdelecroix

And in the doctest for float it would actually be good to also test int and Fraction (from the Python module fractions).

comment:36 Changed 2 years ago by jipilab

  • Status changed from needs_review to needs_work

Some MIP's tests are broken by the code. I will look into this and correct the docstring too.

comment:37 Changed 2 years ago by mforets

  • Authors changed from Jean-Philippe Labbé, Vincent Delecroix to Jean-Philippe Labbé, Vincent Delecroix, Marcelo Forets
  • Branch changed from u/jipilab/22605 to u/mforets/22605
  • Cc tmonteil added
  • Commit changed from fc5089dcb4d33ca2e7b0595a57d8cbc4dccdd733 to c58f8b2b1c097d7885cd2547f61692b2428d0a95
  • Milestone changed from sage-7.6 to sage-8.1
  • Status changed from needs_work to needs_info

hi,

i've adjusted the doctests in MIP and added the int construction test to JP's branch.

i had trouble with Python's Fraction though:

sage: from fractions import Fraction
sage: Polyhedron(vertices=[[Fraction(int(8), int(6))]])
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-27-199a9229f10f> in <module>()
----> 1 Polyhedron(vertices=[[Fraction(int(Integer(8)), int(Integer(6)))]])

 
/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/geometry/polyhedron/constructor.pyc in Polyhedron(vertices, rays, lines, ieqs, eqns, ambient_dim, base_ring, minimi
ze, verbose, backend)
    497     # real numbers
    498     if base_ring not in Rings() or not RDF.has_coerce_map_from(base_ring):
--> 499         raise ValueError("invalid base ring")
    500
    501     # TODO: remove this hack
 
ValueError: invalid base ring

any ideas?


New commits:

1732593Merge branch 'develop' into t/22605/22605
06bb9caMerge branch 'develop' into t/22605/22605
c58f8b2fix docstring, test int, add QQ example

comment:38 Changed 2 years ago by vdelecroix

I think that those two functions should be fixed to work with Fraction

sage: from sage.structure.coerce import py_scalar_to_element, py_scalar_parent
sage: f = fractions.Fraction(2r, 3r)
sage: py_scalar_parent(type(f))    # should be QQ
sage: py_scalar_to_element(f)      # should be Rational
Fraction(2, 3)

(could be a dependency ticket)

comment:39 Changed 2 years ago by vdelecroix

  • Dependencies set to #23345

see #23345

comment:40 Changed 2 years ago by git

  • Commit changed from c58f8b2b1c097d7885cd2547f61692b2428d0a95 to 59528547d173d803f732b2f1cd8315ac09ceb50c

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

5952854add fraction example

comment:41 Changed 2 years ago by jipilab

  • Status changed from needs_info to needs_review

Hi all,

I believe that the last issues have been addressed by Marcelo, so I set this to "needs review".

What is the error given by the bot? A time-out? Yet again, some unknown behavior to me.

comment:42 Changed 2 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Two problems from the doctest need to be fixed

$ sage -t geometry/polyhedron/constructor.py
Running doctests with ID 2017-07-13-19-29-08-cb0892b4.
Git branch: 22605-8.0.rc1
Using --optional=bliss,lrslib,mpir,pandoc_attributes,pandocfilters,python2,rst2ipynb,sage
Doctesting 1 file.
sage -t --warn-long 75.0 geometry/polyhedron/constructor.py
**********************************************************************
File "geometry/polyhedron/constructor.py", line 443, in sage.geometry.polyhedron.constructor.?
Failed example:
    Polyhedron(vertices=[[f]])
Exception raised:
    Traceback (most recent call last):
    ...
    ValueError: invalid base ring
**********************************************************************
1 item had failures:
   1 of  27 in sage.geometry.polyhedron.constructor.?
    [59 tests, 1 failure, 0.55 s]
----------------------------------------------------------------------
sage -t --warn-long 75.0 geometry/polyhedron/constructor.py  # 1 doctest failed
----------------------------------------------------------------------
$ sage -t categories/finite_coxeter_groups.py
Running doctests with ID 2017-07-13-19-30-14-e608e41e.
Git branch: 22605-8.0.rc1
Using --optional=bliss,lrslib,mpir,pandoc_attributes,pandocfilters,python2,rst2ipynb,sage
Doctesting 1 file.
sage -t --warn-long 75.0 categories/finite_coxeter_groups.py
**********************************************************************
File "categories/finite_coxeter_groups.py", line 720, in sage.categories.finite_coxeter_groups.FiniteCoxeterGroups.ParentMethods.permutahedron
Failed example:
    W.permutahedron()
Exception raised:
    Traceback (most recent call last):
    ...
    ValueError: invalid base ring
**********************************************************************
1 item had failures:
   1 of   6 in sage.categories.finite_coxeter_groups.FiniteCoxeterGroups.ParentMethods.permutahedron
    [151 tests, 1 failure, 5.21 s]
----------------------------------------------------------------------
sage -t --warn-long 75.0 categories/finite_coxeter_groups.py  # 1 doctest failed
----------------------------------------------------------------------

comment:43 Changed 2 years ago by vdelecroix

The following comment in the test block Check that giving ``float`` and ``int`` input gets converted to `\RDF` should be modified since int are converted to elements of ZZ and not RDF. Furthermore, replace `\RDF` by ``RDF``.

comment:44 Changed 2 years ago by git

  • Commit changed from 59528547d173d803f732b2f1cd8315ac09ceb50c to 6be64b1450da31f2cc955ba74d7203ebe2ce3bec

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

83bf93223345: conversions from Fraction to Rational
ce7fb2523345: make it faster
e238c9eaddress reviewer's comments
e80700aMerge branch 't/23345/23345' into t/22605/22605
6be64b1fix docstring for int

comment:45 Changed 2 years ago by mforets

  • the base ring is UniversalCyclotomicField(), and base_ring not in Rings() is False
  • RDF.has_coerce_map_from(UniversalCyclotomicField()) is also False

comment:46 follow-up: Changed 2 years ago by vdelecroix

The universal cyclotomic field is the complex field generated by all roots of unity! We don't want to deal with that in polyhedron constructor (as we don't for QQbar). For both QQbar and UCF we can either try conversion to AA or raise an error. Which one do you prefer?

comment:47 in reply to: ↑ 46 ; follow-up: Changed 2 years ago by mforets

Replying to vdelecroix:

The universal cyclotomic field is the complex field generated by all roots of unity! We don't want to deal with that in polyhedron constructor (as we don't for QQbar). For both QQbar and UCF we can either try conversion to AA or raise an error. Which one do you prefer?

Ok, thanks for the clarification. i look forward to suggestions on that choice (since i don't have one).

comment:48 in reply to: ↑ 47 Changed 2 years ago by vdelecroix

Replying to mforets:

Replying to vdelecroix:

The universal cyclotomic field is the complex field generated by all roots of unity! We don't want to deal with that in polyhedron constructor (as we don't for QQbar). For both QQbar and UCF we can either try conversion to AA or raise an error. Which one do you prefer?

Ok, thanks for the clarification. i look forward to suggestions on that choice (since i don't have one).

I think an error is safer. The code in Coxeter group that constructs the polytope should call Polyhedron with an appropriate base ring (like AA or QQ[cos(pi/q)]).

comment:49 Changed 2 years ago by vdelecroix

The following does work

sage: W = CoxeterGroup(['I',7])
sage: n = W.one().canonical_matrix().rank()
sage: weights = W.fundamental_weights()
sage: point = [ZZ.one()] * n
sage: v = sum(point[i-1] * weights[i] for i in weights.keys())
sage: vertices = [(v*w).change_ring(AA) for w in W]
sage: Polyhedron(vertices)

It is perhaps not optimal, but fixes the issue with Coxeter groups.

comment:50 Changed 2 years ago by git

  • Commit changed from 6be64b1450da31f2cc955ba74d7203ebe2ce3bec to 128235e58c243925a51dfc5d07b18c24d2894eb5

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

128235econvert UCF and QQbar to AA in permutahedron

comment:51 Changed 2 years ago by mforets

  • Status changed from needs_work to needs_review

comment:52 follow-up: Changed 2 years ago by mforets

disclaimer: i didn't run those tests which require the optional GAP3 package 'chevie'. if someone has it installed already, could you try it please?

comment:53 in reply to: ↑ 52 Changed 2 years ago by vdelecroix

  • Cc tscrim added

Replying to mforets:

disclaimer: i didn't run those tests which require the optional GAP3 package 'chevie'. if someone has it installed already, could you try it please?

Travis: After this ticket, it will be forbidden to use UCF and QQbar as ground fields for polytopes. I believe that you have GAP3 installed. Could you check that we do not break anything for generating the polyhedra related to Coxeter groups?

comment:54 Changed 2 years ago by mforets

i was getting:

sage: W = ReflectionGroup(['A',3])
...
ImportError: the GAP3 package 'chevie' is needed to work with (complex) reflection groups
sage: is_package_installed("gap3")
False

and got scared with the message below.. but installing turned out to be very fast!

=========================== WARNING ===========================
You are about to download and install the experimental package
gap3-01may17.  This probably won't work at all for you! There
is no guarantee that it will build correctly, or behave as
expected. Use at your own risk!
===============================================================
...

anyway, testing with --optional=gap3 flag i do find problems. those tests with the ReflectionGroup and integer base ring were failing.

in the new commit, i'm 'hardcoding' the base ring that is passed to the constructor to be QQ in those cases (before, this was guessed from the vertices). what do you think?

Last edited 2 years ago by mforets (previous) (diff)

comment:55 Changed 2 years ago by git

  • Commit changed from 128235e58c243925a51dfc5d07b18c24d2894eb5 to 579e33f22d5a5674b6196ba6fc3da53dae5b9aa6

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

579e33ftransform to QQ ring for reflection groups

comment:56 Changed 2 years ago by mforets

with respect to the tests, it also works to infer the base ring from the sum over weights v, that is:

if base_ring is None:
    base_ring = v.base_ring()

instead of

if base_ring is None:
    base_ring = self.base_ring()
    if base_ring is ZZ:
        base_ring = QQ

comment:57 Changed 2 years ago by vdelecroix

  • Status changed from needs_review to needs_work

@Marcelo: this beahvior is bad

sage: W = CoxeterGroup(['I',7])
sage: W.permutahedron(base_ring=QQbar).base_ring() is QQbar
False

If the user supplies explicitely a base_ring in the method permutahedron() then it should be used. The above example should be an error (that would be raised by the polyhedron constructor). The automatic conversion QQbar -> AA and UCF -> AA should only be done when the user calls permutahedron() without specifying the base ring.

comment:58 Changed 2 years ago by git

  • Commit changed from 579e33f22d5a5674b6196ba6fc3da53dae5b9aa6 to 6f6d0f35bd8fda02e538e3ead1a8dd5e3ec5ef33

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

6f6d0f3fix behaviour with QQbar and simplify it

comment:59 Changed 2 years ago by mforets

  • Status changed from needs_work to needs_review

comment:60 Changed 2 years ago by vdelecroix

Here

+            if base_ring is None and v.base_ring() in [UniversalCyclotomicField(), QQbar]:
+                    vertices = [(v*w).change_ring(AA) for w in self]

you have an identation with 8 spaces!

comment:61 Changed 2 years ago by vdelecroix

And concerning the same piece of code, it would be simpler to do

vertices = [v*w for w in self]
if base_ring is None and v.base_ring() in [UniversalCyclotomicField(), QQbar]:
    vertices = [v.change_ring(AA) for v in vertices]
    base_ring = AA

comment:62 follow-up: Changed 2 years ago by git

  • Commit changed from 6f6d0f35bd8fda02e538e3ead1a8dd5e3ec5ef33 to af03de82ed07686322db70895a0ab34973888981

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

af03de8fix indentation issue, and simplify

comment:63 in reply to: ↑ 62 Changed 2 years ago by mforets

Replying to git:

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

af03de8fix indentation issue, and simplify

@vdelec: you're right, thanks.

comment:64 Changed 2 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

This is a great improvement! Thank you!

comment:65 Changed 2 years ago by vbraun

  • Branch changed from u/mforets/22605 to af03de82ed07686322db70895a0ab34973888981
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.