Opened 3 years ago
Closed 3 years ago
#22605 closed defect (fixed)
Better error handling of the polyhedron constructor for nonembedded NumberField and floats.
Reported by:  jipilab  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  geometry  Keywords:  polyhedron, base ring 
Cc:  moritz, mkoeppe, vdelecroix, novoselt, tmonteil, tscrim  Merged in:  
Authors:  JeanPhilippe 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 )
These error messages are not really informative of what the problem really is:
sage: K = NumberField(x^22,'s') sage: s = K.0 sage: L = NumberField(x^32,'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 followup ticket that should be fixed easily from the present resolution.
Change History (65)
comment:1 Changed 3 years ago by
 Description modified (diff)
comment:2 Changed 3 years ago by
 Cc novoselt added
comment:3 Changed 3 years ago by
comment:4 Changed 3 years ago by
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 followup: ↓ 6 Changed 3 years ago by
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
So how to check for the embedding? Simply wrap?
comment:7 Changed 3 years ago by
 Branch set to u/jipilab/22605
comment:8 Changed 3 years ago by
 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:
8678e17  Made support for floats

comment:9 Changed 3 years ago by
 Commit changed from 8678e1784b9bd7bbce8baa84e8846ab8d96649da to 47d9425ef1283708b7a4ce0a7e34a93c2d16b2dc
comment:10 Changed 3 years ago by
 Status changed from new to needs_review
comment:11 Changed 3 years ago by
 Commit changed from 47d9425ef1283708b7a4ce0a7e34a93c2d16b2dc to 9567999c707cc3defd2d416cfadcae6f148937a6
Branch pushed to git repo; I updated commit sha1. New commits:
9567999  pep8 and fix a typo

comment:12 Changed 3 years ago by
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
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
 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:
97694dd  22605: simpler constructor for Polyhedron

comment:15 Changed 3 years ago by
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
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 followup: ↓ 22 Changed 3 years ago by
 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 followup: ↓ 19 Changed 3 years ago by
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
Replying to mkoeppe:
Overall I think the distinction of
base_ring=QQ
andbase_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 followup: ↓ 21 Changed 3 years ago by
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
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 ; followup: ↓ 23 Changed 3 years ago by
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 thebase_ring=ZZ
, then it would convert it automatically toH
representation that is given really describes a lattice polytope and would like to specify its parent to be Polyhedron_ZZ. So forcing thebase_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 ; followup: ↓ 25 Changed 3 years ago by
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 thebase_ring=ZZ
, then it would convert it automatically toH
representation that is given really describes a lattice polytope and would like to specify its parent to be Polyhedron_ZZ. So forcing thebase_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
 Status changed from needs_work to needs_review
comment:25 in reply to: ↑ 23 Changed 3 years ago by
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 thebase_ring=ZZ
, then it would convert it automatically toH
representation that is given really describes a lattice polytope and would like to specify its parent to be Polyhedron_ZZ. So forcing thebase_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 3 years ago by
comment:27 Changed 3 years ago by
 Branch changed from u/vdelecroix/22605 to u/jipilab/22605
comment:28 Changed 3 years ago by
 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:
dc0e524  Rebased on Sage8.0.beta3

comment:29 Changed 3 years ago by
 Commit changed from dc0e5244f046105e39281c6af82f60aeb3a94440 to fc5089dcb4d33ca2e7b0595a57d8cbc4dccdd733
comment:30 Changed 3 years ago by
Tests passed on base.py and constructor.py with Sage8.0.beta3.
The bots didn't have a look at this yet?!
comment:31 Changed 3 years ago by
The bots are not testing many things see https://github.com/sagemath/sagepatchbot/issues/76
comment:32 Changed 3 years ago by
(I just launched mine manually)
comment:33 Changed 3 years ago by
 Description modified (diff)
comment:34 Changed 3 years ago by
.. TESTS:
should be TESTS:
(no ..
)
comment:35 Changed 3 years ago by
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 3 years ago by
 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 3 years ago by
 Branch changed from u/jipilab/22605 to u/mforets/22605
 Cc tmonteil added
 Commit changed from fc5089dcb4d33ca2e7b0595a57d8cbc4dccdd733 to c58f8b2b1c097d7885cd2547f61692b2428d0a95
 Milestone changed from sage7.6 to sage8.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) <ipythoninput27199a9229f10f> in <module>() > 1 Polyhedron(vertices=[[Fraction(int(Integer(8)), int(Integer(6)))]]) /Users/forets/sagesrc/sage/local/lib/python2.7/sitepackages/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:
1732593  Merge branch 'develop' into t/22605/22605

06bb9ca  Merge branch 'develop' into t/22605/22605

c58f8b2  fix docstring, test int, add QQ example

comment:38 Changed 3 years ago by
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:40 Changed 3 years ago by
 Commit changed from c58f8b2b1c097d7885cd2547f61692b2428d0a95 to 59528547d173d803f732b2f1cd8315ac09ceb50c
Branch pushed to git repo; I updated commit sha1. New commits:
5952854  add fraction example

comment:41 Changed 3 years ago by
 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 timeout? Yet again, some unknown behavior to me.
comment:42 Changed 3 years ago by
 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 20170713192908cb0892b4. Git branch: 226058.0.rc1 Using optional=bliss,lrslib,mpir,pandoc_attributes,pandocfilters,python2,rst2ipynb,sage Doctesting 1 file. sage t warnlong 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 warnlong 75.0 geometry/polyhedron/constructor.py # 1 doctest failed 
$ sage t categories/finite_coxeter_groups.py Running doctests with ID 20170713193014e608e41e. Git branch: 226058.0.rc1 Using optional=bliss,lrslib,mpir,pandoc_attributes,pandocfilters,python2,rst2ipynb,sage Doctesting 1 file. sage t warnlong 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 warnlong 75.0 categories/finite_coxeter_groups.py # 1 doctest failed 
comment:43 Changed 3 years ago by
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 3 years ago by
 Commit changed from 59528547d173d803f732b2f1cd8315ac09ceb50c to 6be64b1450da31f2cc955ba74d7203ebe2ce3bec
comment:45 Changed 3 years ago by
 comment:42, doctest in
constructor.py
, done (upon merging with #23345)  comment:42, doctest in
finite_coxeter_groups.py
: i'm confused. the UniversalCyclotomicField? should still be supported in the constructor, right? in this case:
 the base ring is
UniversalCyclotomicField()
, andbase_ring not in Rings()
is False RDF.has_coerce_map_from(UniversalCyclotomicField())
is also False
 comment:43, done
comment:46 followup: ↓ 47 Changed 3 years ago by
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 ; followup: ↓ 48 Changed 3 years ago by
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 bothQQbar
andUCF
we can either try conversion toAA
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 3 years ago by
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 bothQQbar
andUCF
we can either try conversion toAA
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 3 years ago by
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[i1] * 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 3 years ago by
 Commit changed from 6be64b1450da31f2cc955ba74d7203ebe2ce3bec to 128235e58c243925a51dfc5d07b18c24d2894eb5
Branch pushed to git repo; I updated commit sha1. New commits:
128235e  convert UCF and QQbar to AA in permutahedron

comment:51 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:52 followup: ↓ 53 Changed 3 years ago by
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 3 years ago by
 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 3 years ago by
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 gap301may17. 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?
comment:55 Changed 3 years ago by
 Commit changed from 128235e58c243925a51dfc5d07b18c24d2894eb5 to 579e33f22d5a5674b6196ba6fc3da53dae5b9aa6
Branch pushed to git repo; I updated commit sha1. New commits:
579e33f  transform to QQ ring for reflection groups

comment:56 Changed 3 years ago by
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 3 years ago by
 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 3 years ago by
 Commit changed from 579e33f22d5a5674b6196ba6fc3da53dae5b9aa6 to 6f6d0f35bd8fda02e538e3ead1a8dd5e3ec5ef33
Branch pushed to git repo; I updated commit sha1. New commits:
6f6d0f3  fix behaviour with QQbar and simplify it

comment:59 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:60 Changed 3 years ago by
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 3 years ago by
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 followup: ↓ 63 Changed 3 years ago by
 Commit changed from 6f6d0f35bd8fda02e538e3ead1a8dd5e3ec5ef33 to af03de82ed07686322db70895a0ab34973888981
Branch pushed to git repo; I updated commit sha1. New commits:
af03de8  fix indentation issue, and simplify

comment:63 in reply to: ↑ 62 Changed 3 years ago by
comment:64 Changed 3 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
This is a great improvement! Thank you!
comment:65 Changed 3 years ago by
 Branch changed from u/mforets/22605 to af03de82ed07686322db70895a0ab34973888981
 Resolution set to fixed
 Status changed from positive_review to closed
Indeed. Do we want to convert
float > RDF
these two are fully compatible?