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)
Change History (10)
comment:1 Changed 11 years ago by
- Milestone set to sage-4.3
comment:2 Changed 11 years ago by
comment:3 Changed 11 years ago by
- Component changed from performance to misc
comment:5 Changed 11 years ago by
- 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.
comment:6 Changed 11 years ago by
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
- 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
- Merged in set to sage-4.3.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
Merged in this order:
- trac_7535-errors-raise.patch --- timdumol: please remember to put your username in your patch. I have merged this patch in your name.
- trac_7535-part2.patch
See also #7532.