Opened 6 years ago

Closed 6 years ago

Last modified 6 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: was
Priority: minor Milestone: sage-3.1.3
Component: number theory Keywords:
Cc: AlexGhitza Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

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 cremona 6 years ago.
3897-minor.patch (1.9 KB) - added by AlexGhitza 6 years ago.
apply after 3897-residue-fields-ZZ.patch
3897-local_data.patch (72.8 KB) - added by cremona 6 years ago.
3897-deprecation.2.patch (1.0 KB) - added by cremona 6 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by mabshoff

  • Milestone set to sage-3.1.2

comment:2 Changed 6 years ago by AlexGhitza

  • Cc AlexGhitza added

comment:3 Changed 6 years ago by cremona

I am fixing this now....

Changed 6 years ago by cremona

comment:4 Changed 6 years ago by cremona

  • Summary changed from bug in local_information due to the lack of residue_field for ZZ to [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 6 years ago by AlexGhitza

apply after 3897-residue-fields-ZZ.patch

comment:5 Changed 6 years ago by AlexGhitza

  • Summary changed from [with patch, needs review] bug in local_information due to the lack of residue_field for ZZ to [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 follow-up: Changed 6 years ago by AlexGhitza

  • Summary changed from [with patch, positive review] bug in local_information due to the lack of residue_field for ZZ to [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 6 years ago by 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 6 years ago by cremona

  • Summary changed from [with patch, needs work] bug in local_information due to the lack of residue_field for ZZ to [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 6 years ago by AlexGhitza

  • Summary changed from [with new patch, needs review] bug in local_information due to the lack of residue_field for ZZ to [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 6 years ago by cremona

comment:10 Changed 6 years ago by 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 6 years ago by AlexGhitza

  • Summary changed from [with new patch, needs work] bug in local_information due to the lack of residue_field for ZZ to [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 follow-ups: Changed 6 years ago by mabshoff

Alex,

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

Cheers,

Michael

Changed 6 years ago by cremona

comment:13 in reply to: ↑ 12 Changed 6 years ago by 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 6 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged all four patches in Sage 3.1.3.alpha1

comment:15 in reply to: ↑ 12 Changed 6 years ago by was

"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 6 years ago by 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 6 years ago by AlexGhitza

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.