#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)
Change History (15)
Changed 11 years ago by
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Summary changed from smallest_integer() is broken to [with patch, needs review] smallest_integer() is broken
Changed 11 years ago by
comment:2 Changed 11 years ago by
- 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.
comment:3 Changed 11 years ago by
- 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
- 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
(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.)
comment:6 follow-up: ↓ 8 Changed 11 years ago by
- 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
- Resolution set to fixed
- Status changed from new to closed
Merged in Sage 3.2.rc1
comment:8 in reply to: ↑ 6 ; follow-up: ↓ 9 Changed 11 years ago by
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
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
- Merged in set to 3.2.rc1
- Reviewers set to David Loeffler
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!)