Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#4392 closed defect (fixed)

[with patch, positive review] smallest_integer() is broken

Reported by: cremona Owned by: cremona
Priority: major Milestone: sage-3.2
Component: number theory Keywords: number field ideal
Cc: m.t.aranes@… Merged in: 3.2.rc1
Authors: John Cremona Reviewers: David Loeffler
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

For number field ideals and fractional ideals, the smallest_integer() function is broken in 2 ways:

sage: K.<a>=QuadraticField(-5)
sage: I=K.ideal(7)
sage: I.smallest_integer()
ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (237, 0))

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

/home/masgaj/PLMS/<ipython console> in <module>()

/local/jec/sage-3.2.alpha1/local/lib/python2.5/site-packages/sage/rings/number_field/number_field_ideal.pyc in smallest_integer(self)
    731                         bound /= p
    732                 self.smallest_integer = ZZ(bound)
--> 733                 return self.__smallest_integer
    734             I,d = self.integral_split() ## self = I/d
    735             n = I.smallest_integer()    ## n/d in self

AttributeError: 'NumberFieldFractionalIdeal' object has no attribute '_NumberFieldIdeal__smallest_integer'
sage: I.smallest_integer
1
sage: I.smallest_integer()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/masgaj/PLMS/<ipython console> in <module>()

TypeError: 'sage.rings.integer.Integer' object is not callable

First: in line 732 of number_field_ideal.py it has self.smallest_integer instead of self.__smallest_integer, so instead of caching the computed value we overwrite the function itself!

Second: the answer is wrong (as the example shows).

I will try to fix this and post a ptach today (Bug Day 2008-10-30).

Attachments (5)

sage-trac4392.patch (4.6 KB) - added by cremona 11 years ago.
sage-trac4392-2.patch (2.5 KB) - added by cremona 11 years ago.
sage-trac4392-3.patch (5.9 KB) - added by cremona 11 years ago.
sage-trac4392-new.patch (6.9 KB) - added by cremona 11 years ago.
Replaces the three earlier patches
sage-4392-typo.patch (890 bytes) - added by davidloeffler 11 years ago.
fixes docstring typo in sage-trac4392-new.patch

Download all attachments as: .zip

Change History (15)

Changed 11 years ago by cremona

Changed 11 years ago by cremona

comment:1 Changed 11 years ago by cremona

  • Summary changed from smallest_integer() is broken to [with patch, needs review] smallest_integer() is broken

The patch sage-trac4392.patch (based on 3.2.alpha1) fixes this by completely reimplementing the function in a simpler way, using linear algebra instead of factorization. Several new doctests have been added. All tests in sage.rings.number_field pass.

The second patch applies after the first and handles the zero ideal properly (with another doctest to prove it!)

Changed 11 years ago by cremona

comment:2 Changed 11 years ago by cremona

  • Summary changed from [with patch, needs review] smallest_integer() is broken to [with new patch, needs review] smallest_integer() is broken

The third patch sage-trac4392-3.patch adds a new rather useful function coordinates() to the class NumberFieldIdeal? which expresses an element of the field as a QQ-linear combination of the integral basis of the ideal. (This does not work for relative ideals, but then what does?). This uses a change-of-basis matrix which is cached. It results in simpler implementations for the _contains_() and is_integral() functions, as well as the smallest_integer() function where this started.

I checked that all doctests in sage/rings/number_field pass.

Changed 11 years ago by cremona

Replaces the three earlier patches

comment:3 Changed 11 years ago by cremona

  • Cc m.t.aranes@… added; ♫ m.t.aranes@… removed

For ease of reviewing, the patch sage-trac4392-new.patch merges and replaces the three previous ones. It applies to 3.2.alpha3.

comment:4 Changed 11 years ago by davidloeffler

  • Summary changed from [with new patch, needs review] smallest_integer() is broken to [with new patch, positive review] smallest_integer() is broken

Looks good to me. I tested it against 3.1.4 and 3.2.rc0, and in both it applies cleanly. I've tested it a bit under 3.1.4 and it passes all doctests in sage/rings/number_field, and the smallest_integer() and coordinates() functions both seem to work as expected.

comment:5 Changed 11 years ago by davidloeffler

(On a closer inspection of the docstrings, I've noticed a typo in the docstring for the "coordinates" method -- "amllest" for "smallest". But that's the only "issue" I can find.)

Changed 11 years ago by davidloeffler

fixes docstring typo in sage-trac4392-new.patch

comment:6 follow-up: Changed 11 years ago by mabshoff

  • Milestone changed from sage-3.2.1 to sage-3.2
  • Summary changed from [with new patch, positive review] smallest_integer() is broken to [with patch, positive review] smallest_integer() is broken

Merged sage-trac4392-new.patch and sage-4392-typo.patch in Sage 3.2.rc1.

John: The folded(?) patch was a diff a not a "proper" mercurial patch, so the credit in the log does not reflect your authorship. Please make sure to export patches.

Cheers,

Michael

comment:7 Changed 11 years ago by mabshoff

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

Merged in Sage 3.2.rc1

comment:8 in reply to: ↑ 6 ; follow-up: Changed 11 years ago by cremona

Replying to mabshoff:

Merged sage-trac4392-new.patch and sage-4392-typo.patch in Sage 3.2.rc1.

John: The folded(?) patch was a diff a not a "proper" mercurial patch, so the credit in the log does not reflect your authorship. Please make sure to export patches.

Sorry about that. I had tried to make it easier, by using mercurial queues to merge my three earlier patches. But clearly I am not doing it right. Every time I uses queues I read the entire documentation from beginning to end, and think I understand it, but clearly I don't since every time I mess it up. John

Cheers,

Michael

comment:9 in reply to: ↑ 8 Changed 11 years ago by mabshoff

Replying to cremona:

Replying to mabshoff:

Merged sage-trac4392-new.patch and sage-4392-typo.patch in Sage 3.2.rc1.

John: The folded(?) patch was a diff a not a "proper" mercurial patch, so the credit in the log does not reflect your authorship. Please make sure to export patches.

Sorry about that. I had tried to make it easier, by using mercurial queues to merge my three earlier patches. But clearly I am not doing it right. Every time I uses queues I read the entire documentation from beginning to end, and think I understand it, but clearly I don't since every time I mess it up. John

Well, I am more concerned about me getting credit for a patch I did not write. The solution to your problem is to export the que patch.

Cheers,

Michael

comment:10 Changed 11 years ago by davidloeffler

  • Authors set to John Cremona
  • Merged in set to 3.2.rc1
  • Reviewers set to David Loeffler
Note: See TracTickets for help on using tickets.