"return NotImplementedError" in ring.pyx #7532

williamstein · 2009-11-26T05:37:36Z

On Wed, Nov 25, 2009 at 7:26 PM, John H Palmieri <jhpalmieri64@gmail.com> wrote:
> 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"?

That's *definitely* a bug.   No question about it.

Addendum: apply the patches trac_7532.patch and trac_7532-rings.patch. Depends on #7535.

Component: elliptic curves

Author: John Cremona

Reviewer: Tim Dumol, John Palmieri

Merged: sage-4.3.2.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/7532

The text was updated successfully, but these errors were encountered:

sagetrac-ylchapuy · 2009-11-26T09:07:47Z

comment:1

grep gives:

./symbolic/expression.pyx: return NotImplementedError
./symbolic/expression_conversions.py: return NotImplementedError("SymPy function '%s' doesn't exist" % f)
./rings/ring.pyx: return NotImplementedError
./modular/arithgroup/congroup_gammaH.py: return NotImplementedError
./geometry/polyhedra.py: return NotImplementedError

sagetrac-ylchapuy · 2009-11-26T09:14:11Z

comment:2

and allowing other error types we get:

./interfaces/gap.py: return RuntimeError, "Error evaluating %s in %s"%(line, self)
./modular/abvar/finite_subgroup.py: return ValueError, "self and other must be in the same ambient Jacobian"
./groups/perm_gps/permgroup_named.py: return ValueError, "Degree must be 2."
./groups/perm_gps/permgroup_named.py: return ValueError, "Degree must be 2."

sagetrac-ylchapuy · 2009-11-26T09:29:49Z

comment:3

and also (my grep has a strange behavior..?):

*./structure/element.pyx: return ArithmeticError, "Multiplicative order of 0 not defined."
*./rings/finite_field_givaro.pyx: return ArithmeticError, "Multiplicative order of 0 not defined."

jhpalmieri · 2009-11-26T15:45:30Z

comment:4

See also #7535.

TimDumol · 2010-01-18T20:14:51Z

Work Issues: Close if #7535 does

jhpalmieri · 2010-01-19T00:50:06Z

comment:6

Given the patches at #7535, the one remaining case of this is the one in ring.pyx. If I make the change from

return NotImplementedError

to

raise NotImplementedError

there, I get doctest failures in the files

	sage -t -long "devel/sage/sage/schemes/elliptic_curves/monsky_washnitzer.py"
	sage -t -long "devel/sage/sage/schemes/elliptic_curves/padics.py"
	sage -t -long "devel/sage/sage/schemes/elliptic_curves/sha_tate.py"

and maybe some others in this directory.

jhpalmieri · 2010-01-19T00:50:06Z

Changed work issues from Close if #7535 does to none

JohnCremona · 2010-01-19T09:55:48Z

comment:7

On my 64-bit ubuntu build of 4.3.1.rc0 with #7535 patches applied, I only get failures in ell_rational_field.py I'll fix that and try again on a 32-bit machine.

JohnCremona · 2010-01-19T12:43:49Z

Apply after the patches at #7535 on 4.3.1.rc0

JohnCremona · 2010-01-19T12:45:46Z

comment:8

Attachment: trac_7532.patch.gz

The attached patch fixes an error in ell_rational_field.py which is the only one I see in that directory after applying the patches at #7535,

John (jhpalmieri), can you post the details of the errors you had on the other 3 files in that directory?

jhpalmieri · 2010-01-19T18:23:40Z

comment:9

Hi John,

I think your patch really belongs on #7535, not here, since it fixes a doctest which was broken by the patches there. My comment was, if I make another change, this time to ring.pyx (see the new patch), then I get the following doctest failures:

$ sage -t -long /Applications/sage/devel/sage/sage/schemes/elliptic_curves/monsky_washnitzer.py 
sage -t -long "devel/sage/sage/schemes/elliptic_curves/monsky_washnitzer.py"
**********************************************************************
File "/Applications/sage/devel/sage/sage/schemes/elliptic_curves/monsky_washnitzer.py", line 1549:
    sage: A = monsky_washnitzer.matrix_of_frobenius(Q, p, M)    # long time
Exception raised:
    Traceback (most recent call last):
      File "/Applications/sage/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/Applications/sage/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/Applications/sage/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_32[66]>", line 1, in <module>
        A = monsky_washnitzer.matrix_of_frobenius(Q, p, M)    # long time###line 1549:
    sage: A = monsky_washnitzer.matrix_of_frobenius(Q, p, M)    # long time
      File "/Applications/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/monsky_washnitzer.py", line 1606, in matrix_of_frobenius
        F0_reduced = reduce_all(Q, p, F0_coeffs, offset)
      File "/Applications/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/monsky_washnitzer.py", line 1023, in reduce_all
        exact_form = reduce_negative(Q, p, coeffs, offset, exact_form)
      File "/Applications/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/monsky_washnitzer.py", line 757, in reduce_negative
        m = helper_matrix(Q).list()
      File "/Applications/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/monsky_washnitzer.py", line 689, in helper_matrix
        return (1/D) * matrix(Q.base_ring(), 3, 3,
      File "element.pyx", line 1326, in sage.structure.element.RingElement.__div__ (sage/structure/element.c:10897)
      File "coerce.pyx", line 707, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:6073)
      File "coerce.pyx", line 1178, in sage.structure.coerce.CoercionModel_cache_maps.get_action (sage/structure/coerce.c:10911)
      File "coerce.pyx", line 1309, in sage.structure.coerce.CoercionModel_cache_maps.discover_action (sage/structure/coerce.c:12070)
      File "coerce.pyx", line 1346, in sage.structure.coerce.CoercionModel_cache_maps.discover_action (sage/structure/coerce.c:12620)
      File "ring.pyx", line 933, in sage.rings.ring.CommutativeRing._pseudo_fraction_field (sage/rings/ring.c:6321)
      File "ring.pyx", line 900, in sage.rings.ring.CommutativeRing.fraction_field (sage/rings/ring.c:6164)
      File "ring.pyx", line 695, in sage.rings.ring.Ring.is_integral_domain (sage/rings/ring.c:5074)
    NotImplementedError
**********************************************************************
File "/Applications/sage/devel/sage/sage/schemes/elliptic_curves/monsky_washnitzer.py", line 1551:
    sage: B                                                     # long time
Expected:
    [1144 + 264*t + 841*t^2 + 1025*t^3 + O(t^4)  176 + 1052*t + 216*t^2 + 523*t^3 + O(t^4)]
    [   847 + 668*t + 81*t^2 + 424*t^3 + O(t^4)   185 + 341*t + 171*t^2 + 642*t^3 + O(t^4)]
Got:
    [ 514  927]
    [ 702 1036]
**********************************************************************
File "/Applications/sage/devel/sage/sage/schemes/elliptic_curves/monsky_washnitzer.py", line 1562:
    sage: B.det()                                               # long time
Expected:
    11 + 484*t^2 + 451*t^3 + O(t^4)
Got:
    209
**********************************************************************
1 items had failures:
   3 of  70 in __main__.example_32
***Test Failed*** 3 failures.
For whitespace errors, see the file /Users/palmieri/.sage//tmp/.doctest_monsky_washnitzer.py
	 [20.6 s]
exit code: 1024

and

sage -t -long "devel/sage/sage/schemes/elliptic_curves/padics.py"
**********************************************************************
File "/Applications/sage/devel/sage/sage/schemes/elliptic_curves/padics.py", line 197:
    sage: E.padic_regulator(5, 10)
Exception raised:
    Traceback (most recent call last):
      File "/Applications/sage/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/Applications/sage/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/Applications/sage/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_2[3]>", line 1, in <module>
        E.padic_regulator(Integer(5), Integer(10))###line 197:
    sage: E.padic_regulator(5, 10)
      File "/Applications/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/padics.py", line 253, in padic_regulator
        height = self.padic_height(p, prec, check_hypotheses=False)
      File "/Applications/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/padics.py", line 659, in padic_height
        sigma = self.padic_sigma(p, adjusted_prec, check_hypotheses=False)
      File "/Applications/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/padics.py", line 1009, in padic_sigma
        f = X.formal_group().differential(N+2)   # f = 1 + ... + O(t^{N+2})
      File "/Applications/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/formal_group.py", line 360, in differential
        x = self.x(prec+1)
      File "/Applications/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/formal_group.py", line 253, in x
        y = self.y(prec)
      File "/Applications/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/formal_group.py", line 303, in y
        y = -1/w + O(t**prec)
      File "element.pyx", line 1326, in sage.structure.element.RingElement.__div__ (sage/structure/element.c:10897)
      File "coerce.pyx", line 707, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:6073)
      File "coerce.pyx", line 1178, in sage.structure.coerce.CoercionModel_cache_maps.get_action (sage/structure/coerce.c:10911)
      File "coerce.pyx", line 1309, in sage.structure.coerce.CoercionModel_cache_maps.discover_action (sage/structure/coerce.c:12070)
      File "coerce.pyx", line 1346, in sage.structure.coerce.CoercionModel_cache_maps.discover_action (sage/structure/coerce.c:12620)
      File "ring.pyx", line 933, in sage.rings.ring.CommutativeRing._pseudo_fraction_field (sage/rings/ring.c:6321)
      File "ring.pyx", line 900, in sage.rings.ring.CommutativeRing.fraction_field (sage/rings/ring.c:6164)
      File "ring.pyx", line 695, in sage.rings.ring.Ring.is_integral_domain (sage/rings/ring.c:5074)
    NotImplementedError
**********************************************************************

plus 45 more failures in padics.py (too many to include here), and I expect failures in sha-tate.py -- currently in the middle of testing and I have to go teach, so I can't wait for it to finish.

jhpalmieri · 2010-01-19T18:24:36Z

Attachment: trac_7532-donotuse.patch.gz

for purposes of illustration only -- causes doctest failures in schemes/elliptic_curves

JohnCremona · 2010-01-19T20:18:58Z

comment:10

OK, I'll move my patch from here to #7535. I'll also take a look at those other files to see if I can work out what is going on.

TimDumol · 2010-01-20T09:41:11Z

Reviewer: Tim Dumol

TimDumol · 2010-01-20T09:41:11Z

comment:11

Positive review: see #7535 comment:7

TimDumol · 2010-01-20T09:41:11Z

Author: John Palmieri

TimDumol · 2010-01-20T09:41:46Z

comment:12

That should have been positive review. Sorry about that.

JohnCremona · 2010-01-20T10:26:01Z

comment:15

I have got to the bottom of (at least) the problem with monsky_washnitser.py. A quantity D is computed which is in (Z/NZ)[[t]] and non-zero, where N is a prime power not a prime. At some point in the code we need 1/D. This crashes (while D^(-1) is OK). IN the inversion, soemone asks if the parent of D is an integral domain which cannot be determined, which raises a run-time error. Before the patch it returned the error, and as it was returning something nonzero that was interpreted by python as "True".

I think I can fix this, but I am still working on it. It will need a new patch -- sorry to have muddled matters by putting onto this ticket a patch which belonged to #7535.

JohnCremona · 2010-01-20T13:08:53Z

Apply instead of previous one to 4.3.1.rc0 (rings.pyx fix)

JohnCremona · 2010-01-20T13:10:48Z

comment:16

Attachment: trac_7532-rings.patch.gz

The patch trac_7532-rings.patch fixes the problem in rings.pyx and all the fallout -- I checked the entire library and it is fine. The diagnosis was exactly as above, and only a few lines needed to be changed!

I am switchingg this to "needs work" and then "needs review", hoping that it can get in on the back of #7535 (just this once!)

jhpalmieri · 2010-01-20T16:38:44Z

Changed reviewer from Tim Dumol to Tim Dumol, John Palmieri

jhpalmieri · 2010-01-20T16:38:44Z

comment:18

Looks good, all tests pass. Thanks for the fix, John.

jhpalmieri · 2010-01-20T16:38:44Z

Changed author from John Palmieri to John Cremona

JohnCremona · 2010-01-20T17:01:34Z

comment:19

Replying to @jhpalmieri:

Looks good, all tests pass. Thanks for the fix, John.

My pleasure. I hope the release manager can see that trac_7532.patch only needs to be applied once, not both here and at #7535!

I only just discovered the joys of "sage -pt n" which makes testing the whole library a breeze: my research machine has 16 processors of which only a few are usually in use, so I put n=10 and a full test only takes 6 minutes and the -long test I did here took only 701s!

sagetrac-mvngu · 2010-01-23T17:45:58Z

comment:20

First I applied patches at #7535 to Sage 4.3.1, then I applied trac_7532-rings.patch. Running doctest on the full Sage library results in the following failure:

[mvngu@sage sage-4.3.1]$ ./sage -t -long devel/sage-main/sage/schemes/elliptic_curves/ell_rational_field.py
sage -t -long "devel/sage-main/sage/schemes/elliptic_curves/ell_rational_field.py"
**********************************************************************
File "/mnt/usb1/scratch/mvngu/release/sage-4.3.1/devel/sage-main/sage/schemes/elliptic_curves/ell_rational_field.py", line 4027:
    sage: E.isogenies_prime_degree(4)
Exception raised:
    Traceback (most recent call last):
      File "/mnt/usb1/scratch/mvngu/release/sage-4.3.1/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/mnt/usb1/scratch/mvngu/release/sage-4.3.1/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/mnt/usb1/scratch/mvngu/release/sage-4.3.1/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_86[14]>", line 1, in <module>
        E.isogenies_prime_degree(Integer(4))###line 4027:
    sage: E.isogenies_prime_degree(4)
      File "/mnt/usb1/scratch/mvngu/release/sage-4.3.1/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_rational_field.py", line 4036, in isogenies_prime_degree
        return isogenies_sporadic_Q(self, l)
      File "/mnt/usb1/scratch/mvngu/release/sage-4.3.1/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_curve_isogeny.py", line 4004, in isogenies_sporadic_Q
        raise ValueError("%s is not prime."%l)
    ValueError: 4 is not prime.

Did I correctly apply the necessary patches? Does this ticket depend on #7535?

jhpalmieri · 2010-01-23T18:30:48Z

comment:21

Please apply trac_7532.patch as well. Does that help? (See the "addendum" in the ticket description.)

sagetrac-mvngu · 2010-01-23T20:03:36Z

Merged: sage-4.3.2.alpha0

sagetrac-mvngu · 2010-01-23T20:03:36Z

comment:22

Thank you for pointing out the addendum, jhpalmieri. Merged in this order:

patches at Errors should be raised, not returned. #7535
trac_7532.patch
trac_7532-rings.patch

williamstein added this to the sage-4.3.2 milestone Nov 26, 2009

williamstein added c: algebra labels Nov 26, 2009

williamstein assigned aghitza Nov 26, 2009

jhpalmieri assigned JohnCremona and unassigned aghitza Jan 19, 2010

jhpalmieri added c: elliptic curves and removed c: algebra labels Jan 19, 2010

JohnCremona added the s: needs review label Jan 19, 2010

JohnCremona added s: needs work and removed s: needs review labels Jan 19, 2010

TimDumol mannequin removed the s: needs work label Jan 20, 2010

TimDumol mannequin closed this as completed Jan 20, 2010

TimDumol mannequin added the s: needs work label Jan 20, 2010

TimDumol mannequin reopened this Jan 20, 2010

TimDumol mannequin added s: needs review and removed s: needs work labels Jan 20, 2010

JohnCremona added s: needs work and removed s: positive review labels Jan 20, 2010

This comment has been minimized.

Sign in to view

jhpalmieri added s: positive review and removed s: needs review labels Jan 20, 2010

sagetrac-mvngu mannequin removed the s: positive review label Jan 23, 2010

sagetrac-mvngu mannequin closed this as completed Jan 23, 2010

jhpalmieri mentioned this issue Jan 19, 2010

Errors should be raised, not returned. #7535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"return NotImplementedError" in ring.pyx #7532

"return NotImplementedError" in ring.pyx #7532

williamstein commented Nov 26, 2009

sagetrac-ylchapuy mannequin commented Nov 26, 2009

sagetrac-ylchapuy mannequin commented Nov 26, 2009

sagetrac-ylchapuy mannequin commented Nov 26, 2009

jhpalmieri commented Nov 26, 2009

TimDumol mannequin commented Jan 18, 2010

jhpalmieri commented Jan 19, 2010

jhpalmieri commented Jan 19, 2010

JohnCremona commented Jan 19, 2010

JohnCremona commented Jan 19, 2010

JohnCremona commented Jan 19, 2010

jhpalmieri commented Jan 19, 2010

jhpalmieri commented Jan 19, 2010

JohnCremona commented Jan 19, 2010

TimDumol mannequin commented Jan 20, 2010

TimDumol mannequin commented Jan 20, 2010

TimDumol mannequin commented Jan 20, 2010

TimDumol mannequin commented Jan 20, 2010

JohnCremona commented Jan 20, 2010

JohnCremona commented Jan 20, 2010

JohnCremona commented Jan 20, 2010

jhpalmieri commented Jan 20, 2010

This comment has been minimized.

jhpalmieri commented Jan 20, 2010

jhpalmieri commented Jan 20, 2010

JohnCremona commented Jan 20, 2010

sagetrac-mvngu mannequin commented Jan 23, 2010

jhpalmieri commented Jan 23, 2010

sagetrac-mvngu mannequin commented Jan 23, 2010

sagetrac-mvngu mannequin commented Jan 23, 2010

"return NotImplementedError" in ring.pyx #7532

"return NotImplementedError" in ring.pyx #7532

Comments

williamstein commented Nov 26, 2009

sagetrac-ylchapuy mannequin commented Nov 26, 2009

sagetrac-ylchapuy mannequin commented Nov 26, 2009

sagetrac-ylchapuy mannequin commented Nov 26, 2009

jhpalmieri commented Nov 26, 2009

TimDumol mannequin commented Jan 18, 2010

jhpalmieri commented Jan 19, 2010

jhpalmieri commented Jan 19, 2010

JohnCremona commented Jan 19, 2010

JohnCremona commented Jan 19, 2010

JohnCremona commented Jan 19, 2010

jhpalmieri commented Jan 19, 2010

jhpalmieri commented Jan 19, 2010

JohnCremona commented Jan 19, 2010

TimDumol mannequin commented Jan 20, 2010

TimDumol mannequin commented Jan 20, 2010

TimDumol mannequin commented Jan 20, 2010

TimDumol mannequin commented Jan 20, 2010

JohnCremona commented Jan 20, 2010

JohnCremona commented Jan 20, 2010

JohnCremona commented Jan 20, 2010

jhpalmieri commented Jan 20, 2010

This comment has been minimized.

jhpalmieri commented Jan 20, 2010

jhpalmieri commented Jan 20, 2010

JohnCremona commented Jan 20, 2010

sagetrac-mvngu mannequin commented Jan 23, 2010

jhpalmieri commented Jan 23, 2010

sagetrac-mvngu mannequin commented Jan 23, 2010

sagetrac-mvngu mannequin commented Jan 23, 2010