Opened 13 years ago
Closed 13 years ago
#7532 closed defect (fixed)
"return NotImplementedError" in ring.pyx
Reported by: | was | Owned by: | cremona |
---|---|---|---|
Priority: | major | Milestone: | sage-4.3.2 |
Component: | elliptic curves | Keywords: | |
Cc: | Merged in: | sage-4.3.2.alpha0 | |
Authors: | John Cremona | Reviewers: | Tim Dumol, John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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.
Attachments (3)
Change History (25)
comment:1 Changed 13 years ago by
comment:2 Changed 13 years ago by
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."
comment:3 Changed 13 years ago by
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."
comment:4 Changed 13 years ago by
See also #7535.
comment:5 Changed 13 years ago by
- Work issues set to Close if #7535 does
comment:6 Changed 13 years ago by
- Component changed from algebra to elliptic curves
- Owner changed from AlexGhitza to cremona
- Work issues Close if #7535 does deleted
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.
comment:7 Changed 13 years ago by
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.
comment:8 Changed 13 years ago by
- Status changed from new to needs_review
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?
comment:9 Changed 13 years ago by
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.
Changed 13 years ago by
for purposes of illustration only -- causes doctest failures in schemes/elliptic_curves
comment:10 Changed 13 years ago by
- Status changed from needs_review to needs_work
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.
comment:11 Changed 13 years ago by
- Resolution set to fixed
- Reviewers set to Tim Dumol
- Status changed from needs_work to closed
Positive review: see comment:7:ticket:7535
comment:12 Changed 13 years ago by
- Status changed from closed to needs_work
That should have been positive review. Sorry about that.
comment:13 Changed 13 years ago by
- Status changed from needs_work to needs_review
comment:14 Changed 13 years ago by
- Status changed from needs_review to positive_review
comment:15 Changed 13 years ago by
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.
comment:16 Changed 13 years ago by
- Status changed from positive_review to needs_work
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!)
comment:17 Changed 13 years ago by
- Status changed from needs_work to needs_review
comment:18 follow-up: ↓ 19 Changed 13 years ago by
- Description modified (diff)
- Reviewers changed from Tim Dumol to Tim Dumol, John Palmieri
- Status changed from needs_review to positive_review
Looks good, all tests pass. Thanks for the fix, John.
comment:19 in reply to: ↑ 18 Changed 13 years ago by
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!
comment:20 Changed 13 years ago by
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?
comment:21 Changed 13 years ago by
Please apply trac_7532.patch as well. Does that help? (See the "addendum" in the ticket description.)
comment:22 Changed 13 years ago by
- Merged in set to sage-4.3.2.alpha0
- Status changed from positive_review to closed
Thank you for pointing out the addendum, jhpalmieri. Merged in this order:
- patches at #7535
- trac_7532.patch
- trac_7532-rings.patch
grep gives: