Opened 14 years ago

Closed 14 years ago

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

Reported by: Owned by: cremona cremona major sage-3.2 number theory number field ideal mtaranes 3.2.rc1 John Cremona David Loeffler N/A

### 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).

### comment:1 Changed 14 years ago by cremona

Summary: smallest_integer() is broken → [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!)

### comment:2 Changed 14 years ago by cremona

Summary: [with patch, needs review] smallest_integer() is broken → [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 14 years ago by cremona

Replaces the three earlier patches

### comment:3 Changed 14 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 14 years ago by davidloeffler

Summary: [with new patch, needs review] smallest_integer() is broken → [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 14 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 14 years ago by davidloeffler

fixes docstring typo in sage-trac4392-new.patch

### comment:6 follow-up:  8 Changed 14 years ago by mabshoff

Milestone: sage-3.2.1 → sage-3.2 [with new patch, positive review] smallest_integer() is broken → [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 14 years ago by mabshoff

Resolution: → fixed new → closed

Merged in Sage 3.2.rc1

### comment:8 in reply to:  6 ; follow-up:  9 Changed 14 years ago by cremona

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 14 years ago by 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 14 years ago by davidloeffler

Authors: → John Cremona → 3.2.rc1 → David Loeffler

### comment:11 Changed 3 weeks ago by chapoton

Cc: mtaranes added; m.t.aranes@… removed → N/A
Note: See TracTickets for help on using tickets.