Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#3897 closed defect (fixed)

[with new patch, positive review] bug in local_information due to the lack of residue_field for ZZ

Reported by: wuthrich Owned by: William Stein
Priority: minor Milestone: sage-3.1.3
Component: number theory Keywords:
Cc: Alex Ghitza Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

E = EllipticCurve([1,1])
E.local_information(3)

yields

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)

/local/pmzcw/prog/sage-3.1.1/<ipython console> in <module>()

/hades/staff/pmzcw/prog/sage/local/lib/python2.5/site-packages/sage/schemes/elliptic_curves/ell_number_field.py in local_information(self, P, proof)
    375         if isinstance(P, RingElement):
    376             P = self.base_ring().ideal(P)
--> 377         return self.integral_model()[0]._tate(P, proof)
    378
    379     def local_minimal_model(self, P, proof = None):

/hades/staff/pmzcw/prog/sage/local/lib/python2.5/site-packages/sage/schemes/elliptic_curves/ell_number_field.py in _tate(self, P, proof)
    517         OK = K.maximal_order()
    518         t = verbose("Running Tate's algorithm with P = %s"%P, level=1)
--> 519         F = OK.residue_field(P)
    520         p = F.characteristic()
    521         if P.is_principal():

AttributeError: 'sage.rings.integer_ring.IntegerRing_class' object has no attribute 'residue_field'

The problem is that ZZ has no object residue_field, while number rings have. Either one should add this function or write local_information separately for curves over QQ.

Attachments (4)

3897-residue-fields-ZZ.patch (25.5 KB) - added by John Cremona 14 years ago.
3897-minor.patch (1.9 KB) - added by Alex Ghitza 14 years ago.
apply after 3897-residue-fields-ZZ.patch
3897-local_data.patch (72.8 KB) - added by John Cremona 14 years ago.
3897-deprecation.2.patch (1.0 KB) - added by John Cremona 14 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 14 years ago by Michael Abshoff

Milestone: sage-3.1.2

comment:2 Changed 14 years ago by Alex Ghitza

Cc: Alex Ghitza added

comment:3 Changed 14 years ago by John Cremona

I am fixing this now....

Changed 14 years ago by John Cremona

comment:4 Changed 14 years ago by John Cremona

Summary: bug in local_information due to the lack of residue_field for ZZ[with patch, needs review] bug in local_information due to the lack of residue_field for ZZ

OK, so the patch 3897-residue-fields-ZZ.patch implements residue fields for ZZ. ALong the way there were quite a few smallish fixes needed in related things, and while I was immersed in number_field.py etc I added a whole lot of docstrings and doctests there.

This applies to 3.1.2.rc1 AFTER the patches for #1951 which are related (they also relate to residue fields).

After this has been accepted (after revision if necessary, of course!) then we can start to work on the original problem: I know that local_information() still does not work for curves defined over Q.

Chris: I have started going through all the functions for elliptic curve over Q vs. curves over number fields, to make everything as consistent as possible. These residue field extensions will help.

Changed 14 years ago by Alex Ghitza

Attachment: 3897-minor.patch added

apply after 3897-residue-fields-ZZ.patch

comment:5 Changed 14 years ago by Alex Ghitza

Summary: [with patch, needs review] bug in local_information due to the lack of residue_field for ZZ[with patch, positive review] bug in local_information due to the lack of residue_field for ZZ

Looks good. There were a few typos which are fixed in 3897-minor.patch.

comment:6 Changed 14 years ago by Alex Ghitza

Summary: [with patch, positive review] bug in local_information due to the lack of residue_field for ZZ[with patch, needs work] bug in local_information due to the lack of residue_field for ZZ

Oops! I take that back. I noticed that there is no doctest checking if the reported bug was fixed, and running that example still does not work (although in a new way). I'll see if I can track down what's happening.

comment:7 in reply to:  6 Changed 14 years ago by John Cremona

Replying to AlexGhitza:

Oops! I take that back. I noticed that there is no doctest checking if the reported bug was fixed, and running that example still does not work (although in a new way). I'll see if I can track down what's happening.

Alex: this patch is intended only to fix the residue field for ZZ issue; now I have done that I am still working on getting local information to work properly. So this is really two tickets.

Could we merge and close this one and open a new one for the local info problem? Or put the ZZ residue fields into a new patch which can be closed right away, with a cross-reference from this one?

comment:8 Changed 14 years ago by John Cremona

Summary: [with patch, needs work] bug in local_information due to the lack of residue_field for ZZ[with new patch, needs review] bug in local_information due to the lack of residue_field for ZZ

OK, despite my previous remark I have finished and the 3rd patch (to be applied after previous) sorts out the original issue. More than that:

  • function _tidy_model() was wrong and has been fixed
  • New file ell_local_data.py defines a new class EllipticCurveLocalData which does the work; code moved to here from ell_number_field (especially the _tate() function implementing Tate's algorithm)
  • A few functions added to integer.pyx and rational.pyx so as to allow common code for QQ and other number fields.
  • Now all functions in ell_number_field.py work on curves defined over QQ in a consistent way.
  • Over QQ, new algorithm option "generic" in conductor() uses the generic number field code (slower, but shows consistency).

comment:9 Changed 14 years ago by Alex Ghitza

Summary: [with new patch, needs review] bug in local_information due to the lack of residue_field for ZZ[with new patch, needs work] bug in local_information due to the lack of residue_field for ZZ

John,

This is good stuff. Unfortunately, your patch does not contain your new file ell_local_data.py. I think you probably forgot to add it to the hg repository before exporting your patch. You should go to devel/sage/sage and do "hg add schemes/elliptic_curves/ell_local_data.py", then refresh your patch 3897-local_data.patch and re-upload it to trac.

Changed 14 years ago by John Cremona

Attachment: 3897-local_data.patch added

comment:10 Changed 14 years ago by John Cremona

OK, I've done that. I'm still getting used to the "hg q" way of doing things, which doesn't have a commit stage -- so while I think the patch is correctly identified as due to me, there is not (I think) the usual one-line description.

Enjoy.

comment:11 Changed 14 years ago by Alex Ghitza

Summary: [with new patch, needs work] bug in local_information due to the lack of residue_field for ZZ[with new patch, positive review] bug in local_information due to the lack of residue_field for ZZ

Positive review. I added another small patch deprecating local_information (since John's last patch renames it to local_data without any comment). Apply all the patches in sequence.

For the record: at the moment, writing any function that deals with number fields involves one of the following unpleasant steps 1) write special code to deal with QQ or 2) add functionality to QQ so that it pretends to be a number field. This leads to code duplication and obfuscation. Also, whenever a bug is fixed or a feature is added to number fields, one has to remember to do the same with QQ. Very sad!

Looking at this situation, it feels very much like QQ should inherit from rings.number_field.NumberField_absolute rather than the current rings.number_field_base.NumberField?. This appears to be difficult to do because of circular imports; I've been thinking about the issue and hope to come up with at least some solution that will allow us to truly treat number fields and QQ on an equal footing.

comment:12 Changed 14 years ago by Michael Abshoff

Alex,

the deprecated routine should still call the new function, i.e. a warning is issues, but the code still works.

Cheers,

Michael

Changed 14 years ago by John Cremona

Attachment: 3897-deprecation.2.patch added

comment:13 in reply to:  12 Changed 14 years ago by John Cremona

Replying to mabshoff:

Alex,

the deprecated routine should still call the new function, i.e. a warning is issues, but the code still works.

Cheers,

Michael

I added a line to do that. Trac would not let me replace Alex's, but it is a replacement.

comment:14 Changed 14 years ago by Michael Abshoff

Resolution: fixed
Status: newclosed

Merged all four patches in Sage 3.1.3.alpha1

comment:15 in reply to:  12 Changed 14 years ago by William Stein

"Looking at this situation, it feels very much like QQ should inherit from rings.number_field.NumberField??_absolute rather than the current rings.number_field_base.NumberField??. This appears to be difficult to do because of circular imports; I've been thinking about the issue and hope to come up with at least some solution that will allow us to truly treat number fields and QQ on an equal footing. "

The original intention was that any functionality that could be implemented in a way that is common to both NumberField_absolute and QQ, should be implemented in rings.number_field.NumberField?. Maybe you should just move *all* such functionality up there? If it can't be implemented there, how will it make sense for QQ?

At least that was how the thinking went a year ago in the original implementation. This may have turned out to be completely wrong. Keep at it until you find the right solution.

comment:16 Changed 14 years ago by John Cremona

I think William is probably right, but still I'm glad that these patches have been merged before we tried to solve everything! The one I did implementing residue fields for ZZ was rather similar. But that's now done!

comment:17 Changed 14 years ago by Alex Ghitza

William,

I'm glad you commented on this. That's exactly the path I had chosen to follow (after discussing it with Craig), so it's good to have confirmation from someone involved in the original design decision.

Note: See TracTickets for help on using tickets.