Opened 11 years ago

Closed 11 years ago

#7535 closed defect (fixed)

Errors should be raised, not returned.

Reported by: SimonKing Owned by: tbd
Priority: critical Milestone: sage-4.3.2
Component: misc Keywords: error return
Cc: Merged in: sage-4.3.2.alpha0
Authors: Tim Dumol Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

The following issue was considered in at least two threads, the latest at http://groups.google.com/group/sage-devel/browse_thread/thread/3661fde739474fdb.

There are several places in the Sage code where errors are not raised but returned. A hopefully exhaustive search brought up the following:

sage: import exceptions
sage: for E in dir(exceptions):
....:     if not E.startswith('__'):
....:         s = search_src("return "+E)
....:         if s:
....:             print s
....:
rings/finite_field_element.py:384:                return ArithmeticError, "Multiplicative order of 0 not defined."
rings/finite_field_givaro.pyx:1956:                return ArithmeticError, "Multiplicative order of 0 not defined."
structure/element.pyx:2601:            return ArithmeticError, "Multiplicative order of 0 not defined."

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)

interfaces/gap.py:580:            return RuntimeError, "Error evaluating %s in %s"%(line, self)

modular/abvar/finite_subgroup.py:280:            return ValueError, "self and other must be in the same ambient Jacobian"
groups/perm_gps/permgroup_named.py:945:            return ValueError, "Degree must be 2."
groups/perm_gps/permgroup_named.py:988:            return ValueError, "Degree must be 2."

Of course, if an error is returned it can't be catched, which implies obvious problems.

I have no idea what component that ticket should be associated with. "Performance" seemed the least inappropriate description to me.

Is there any of the above cases in which the error should really be returned, not raised?

Attachments (2)

trac_7535-errors-raise.patch (6.1 KB) - added by timdumol 11 years ago.
Makes all remaining returns of exceptions into raising.
trac_7535-part2.patch (1.2 KB) - added by jhpalmieri 11 years ago.
apply on top of the other patch

Download all attachments as: .zip

Change History (10)

comment:1 Changed 11 years ago by SimonKing

  • Milestone set to sage-4.3

comment:2 Changed 11 years ago by jhpalmieri

See also #7532.

comment:3 Changed 11 years ago by timdumol

  • Component changed from performance to misc

comment:4 Changed 11 years ago by timdumol

  • Status changed from new to needs_review

This should do the trick.

Changed 11 years ago by timdumol

Makes all remaining returns of exceptions into raising.

comment:5 Changed 11 years ago by jhpalmieri

  • Authors set to Tim Dumol
  • Reviewers set to John Palmieri

I'm not sure what you mean by "remaining", since there is no patch at #7532 (or elsewhere?) fixing any other instances of this. I'm attaching a patch dealing with two more of these, leaving, I think, just the one in rings.pyx. See #7532 for that one.

Positive review for timdumol's patch, so if mine is okay, this whole ticket can get a positive review.

Changed 11 years ago by jhpalmieri

apply on top of the other patch

comment:6 Changed 11 years ago by cremona

I am about to add a patch to #7532 which fixes (for me) the remaining issue in schemes/elliptic_curves.

comment:7 Changed 11 years ago by timdumol

  • Status changed from needs_review to positive_review

Doctests pass with the latter patch and the one in #7532, so I'll mark both as positive review.

comment:8 Changed 11 years ago by mvngu

  • Merged in set to sage-4.3.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

Merged in this order:

  1. trac_7535-errors-raise.patch --- timdumol: please remember to put your username in your patch. I have merged this patch in your name.
  2. trac_7535-part2.patch
Note: See TracTickets for help on using tickets.