should you ever return an error?

[John H Palmieri]

In ring.pyx, there is code like this:

if proof:
return NotImplementedError
else:
return False

I would think that the second line should say "raise
NotImplementedError". (Changing it makes some doctests fail,
though.) Is there a good reason for doing "return
NotImplementedError"?

--
John

[Rob Beezer]

Hi John,

Hard to be certain without the context, but I think this is the
relevant discussion:
http://groups.google.com/group/sage-devel/browse_thread/thread/dc104b7cb64d39e3/

In short, IIRC, sometimes a construction will create a ring. In
certain instances the construction might also be a field. Maybe it is
easy to know this (perhaps certain parameters establish the certainty
of this), while in other constructions it may be difficult (or
expensive) to know if the ring constructed is a field or not.

You might care about this if you have an algorithm that would be
faster if you were certain the input is a field, and not just a ring.
So I think this construct is there to handle this sort of situation.
But read the discussion referenced for the details, because I'm pretty
sure I'm not summarizing it properly.

Rob

[Dan Drake]

AFAIK, *returning* a NotImplementedError totally defeats the purpose of
try/except clauses, since then they will never see the error! Consider

try:
x = something_possibly_not_implemented
except NotImplementedError:
x = do_it_another_way

x would always get a NotImplementedError as its value!

Dan

--
--- Dan Drake
----- http://mathsci.kaist.ac.kr/~drake
-------

[William Stein]

That's *definitely* a bug (almost certainly my fault). No question
about it. I made this #7532:

http://trac.sagemath.org/sage_trac/ticket/7532

It sounds like you have a fix already, so just let us know when there
is a patch up to review.

Thanks!

William

[Florent Hivert]

Hi there,

I suspected that this could be a common mistake. So I did a grep... It seems I
was right...

Florent

tomahawk-*ge-combinat/sage $ grep return.\*Error **/*.py*
categories/enumerated_sets.py: ``EnumeratedSet()`` it returns `NotImplementedError` since one does
geometry/polyhedra.py: return NotImplementedError
groups/perm_gps/permgroup_named.py: return ValueError, "Degree must be 2."
groups/perm_gps/permgroup_named.py: return ValueError, "Degree must be 2."
groups/perm_gps/permgroup.py: return TypeError, "Group must be simple."
interfaces/gap.py: return RuntimeError, "Error evaluating %s in %s"%(line, self)
libs/pari/gen.pyx: return "PariError(%d)"%self.args[0]
misc/latex.py: return "Error"
misc/latex.py: return "Error"
misc/latex.py: return "Error"
misc/latex.py: return "Error"
misc/latex.py: return "Error: dvipng failed."
misc/latex.py: return "Error latexing slide."
misc/typecheck.py:__all__ = ["takes", "InputParameterError", "returns", "ReturnValueError"]
modular/abvar/finite_subgroup.py: return ValueError, "self and other must be in the same ambient Jacobian"
modular/arithgroup/congroup_gammaH.py: return NotImplementedError
modular/hecke/module.py: abstract base class, return NotImplementedError.
rings/finite_field_element.py: return ArithmeticError, "Multiplicative order of 0 not defined."
rings/finite_field_givaro.pyx: return ArithmeticError, "Multiplicative order of 0 not defined."
rings/number_field/number_field.py: If this function returns True (and doesn't raise a ValueError),
rings/ring.pyx: return NotImplementedError
rings/ring.pyx: The following example returns a ``NotImplementedError`` since the
server/notebook/twist.py: return HTMLResponse(stream = 'Error in introspection -- invalid cell id.')
server/notebook/worksheet.py: return "print 'Error loading %s -- file not found'"%filename
server/notebook/worksheet.py: return "print r'''Error compiling cython file:\n%s'''"%msg
structure/element.pyx: return ArithmeticError, "Multiplicative order of 0 not defined."
structure/factorization.py: This method will return a TypeError if the coercion is not
symbolic/expression_conversions.py: return NotImplementedError("SymPy function '%s' doesn't exist" % f)
symbolic/expression.pyx: return NotImplementedError

[Robert Bradshaw]

Though all of the above look like errors to me, not that there is the
special value NotImplemented that can be *returned* in certain cases

http://docs.python.org/library/constants.html

- Robert

[Simon King]

Hi Robert,

On Nov 26, 8:41 am, Robert Bradshaw <rober...@math.washington.edu>
wrote:
[...]

> Though all of the above look like errors to me, not that there is the  
> special value NotImplemented that can be *returned* in certain cases
>
> http://docs.python.org/library/constants.html

But NotImplementedError is a Python object, and (as any object) it can
be returned.

Actually I reported a similar bug (when an error was returned instead
of raised) before, and I think it got fixed. But apparently we forgot
to systematically search for "return *Error".

sage: search_src('return NotImplementedError')
rings/ring.pyx:687: return NotImplementedError
modular/hecke/module.py:706: abstract base class, return
NotImplementedError.
modular/arithgroup/congroup_gammaH.py:928: return
NotImplementedError
geometry/polyhedra.py:1068: return NotImplementedError
symbolic/expression.pyx:1524: return NotImplementedError
symbolic/expression_conversions.py:638: return

NotImplementedError("SymPy function '%s' doesn't exist" % f)

sage: search_src('return RuntimeError')
interfaces/gap.py:580: return RuntimeError, "Error

evaluating %s in %s"%(line, self)

etc.

I think I will create a ticket for it.

Cheers,
Simon

[Robert Bradshaw]

On Nov 26, 2009, at 1:06 AM, Simon King wrote:

> Hi Robert,
>
> On Nov 26, 8:41 am, Robert Bradshaw <rober...@math.washington.edu>
> wrote:
> [...]
>> Though all of the above look like errors to me, not that there is the
>> special value NotImplemented that can be *returned* in certain cases
>>
>> http://docs.python.org/library/constants.html
>
> But NotImplementedError is a Python object, and (as any object) it can
> be returned.

Yes, I understand that it can be returned, but it almost always should
be raised. My point (which was somewhat tangential) is that there is
an object NotImplemented that's meant to be returned, not raised,
under normal circumstances.

- Robert

[Simon King]

Hi Robert,

On Nov 26, 9:16 am, Robert Bradshaw <rober...@math.washington.edu>

wrote:
> > On Nov 26, 8:41 am, Robert Bradshaw <rober...@math.washington.edu>
> > wrote:
> > [...]
> >> Though all of the above look like errors to me, not that there is the
> >> special value NotImplemented that can be *returned* in certain cases

Oops, I did not auto-correct your misspelling. Now I see that you
meant to write "*note* that there is ...".

I did not know that NotImplemented constant.

However, there are several errors that are returned. See ticket
http://trac.sagemath.org/sage_trac/ticket/7535

Cheers,
Simon

[Martin Albrecht]

Note that over allocating has a performance hit attached to it:

sage: P = PolynomialRing(QQ,500,'x')
sage: f = P.random_element()
sage: R = PolynomialRing(QQ,1000,'x')
sage: g = R(f)

sage: %timeit f*f
100000 loops, best of 3: 18.2 µs per loop

sage: %timeit g*g
10000 loops, best of 3: 32.3 µs per loop

Also, it does increases the memory requirements for each and every element.


Martin

--
name: Martin Albrecht
_pgp: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x8EF0DC99
_otr: 47F43D1A 5D68C36F 468BAEBA 640E8856 D7951CCF
_www: http://www.informatik.uni-bremen.de/~malb
_jab: martinr...@jabber.ccc.de

[Martin Albrecht]

Sorry, wrong thread.