Opened 4 years ago

Last modified 4 years ago

#24455 new defect

_integer_ for QQbar and QQ raise different errors.

Reported by: mcbell Owned by:
Priority: major Milestone: sage-8.2
Component: number fields Keywords: TypeError, ValueError, ZZ, QQbar
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: u/mcbell/_integer__for_qqbar_and_qq_raise_different_errors_ (Commits, GitHub, GitLab) Commit: b4df150c788cad61752ce513c726215c23013970
Dependencies: Stopgaps:

Status badges

Description

sage: ZZ(1/2)

fails and (correctly) raises a TypeError? while

sage: ZZ(QQbar(sqrt(17)))

fails and (incorrectly) raises a ValueError?.

This has several knock on effects, in particular vector(ZZ, [list of algebraic numbers]) all of whoms try / except blocks are set up to catch a TypeError? in the event that one of the elements fails to be coerced to an integer. This in turn causes Polyhedron([(AA(sqrt(17)),)]).integral_points() to fail as this ValueError? is never caught.

Change History (8)

comment:1 follow-up: Changed 4 years ago by vdelecroix

I believe that ValueError for ZZ(1/2) is the correct one. The main reason is that the following conversion should work ZZ(QQ(3)). Hence, it is not due to the type of the object but its value.

Note that the following coercion should fail with a TypeError ZZ.coerce(QQ(3)).

comment:2 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by mcbell

Replying to vdelecroix:

I believe that ValueError for ZZ(1/2) is the correct one. The main reason is that the following conversion should work ZZ(QQ(3)). Hence, it is not due to the type of the object but its value.

Note that the following coercion should fail with a TypeError ZZ.coerce(QQ(3)).

Hmm, so does that mean that the Sequence initialiser in sage/structure/sequence.pyc should catch more types of errors since conversion can also fail due to a ValueError??

    455             for i in range(len(x)):
    456                 try:
    457                     x[i] = universe(x[i])
    458                 except TypeError:
    459                     raise TypeError("unable to convert {} to an element of {}"

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

Replying to mcbell:

Replying to vdelecroix:

I believe that ValueError for ZZ(1/2) is the correct one. The main reason is that the following conversion should work ZZ(QQ(3)). Hence, it is not due to the type of the object but its value.

Note that the following coercion should fail with a TypeError ZZ.coerce(QQ(3)).

Hmm, so does that mean that the Sequence initialiser in sage/structure/sequence.pyc should catch more types of errors since conversion can also fail due to a ValueError??

    455             for i in range(len(x)):
    456                 try:
    457                     x[i] = universe(x[i])
    458                 except TypeError:
    459                     raise TypeError("unable to convert {} to an element of {}"

Two possibilities

  1. (line 458) except TypeError -> except (TypeError, ValueError)
  2. (line 457) x[i] = universe.coerce(x[i])

I do prefer the second one since coercion is stricter and more predictible.

comment:4 in reply to: ↑ 3 Changed 4 years ago by mcbell

Replying to vdelecroix:

Two possibilities

  1. (line 458) except TypeError -> except (TypeError, ValueError)
  2. (line 457) x[i] = universe.coerce(x[i])

I do prefer the second one since coercion is stricter and more predictible.

I'm not sure that we can use the second option since we still want something like:

vector(ZZ, [7, 3/1])

to succeed. However I think the first would work perfectly well.

comment:5 Changed 4 years ago by mcbell

  • Branch set to u/mcbell/_integer__for_qqbar_and_qq_raise_different_errors_

comment:6 Changed 4 years ago by git

  • Commit set to b4df150c788cad61752ce513c726215c23013970

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

b4df150Sequence now also catches ValueError.

comment:7 follow-up: Changed 4 years ago by jdemeyer

This is not the right fix. _integer_ should raise TypeError instead.

comment:8 in reply to: ↑ 7 Changed 4 years ago by mcbell

Replying to jdemeyer:

This is not the right fix. _integer_ should raise TypeError instead.

So I certainly agree that Rational._integer_ (which raises a TypeError?) and AlgebraicNumber?._integer_ (which raises a ValueError?) should raise the same error. However I'm not sure why AlgebraicNumber?._integer_ should be changed to raise a TypeError?. I would have thought that a ValueError? (https://docs.python.org/2/library/exceptions.html#exceptions.ValueError):

Raised when a built-in operation or function receives an argument that has the right type but an inappropriate value, and the situation is not described by a more precise exception such as IndexError?.

would be the appropriate error for both, similar to how int('foo') raises a ValueError?.

Note: See TracTickets for help on using tickets.