#9359 closed enhancement (fixed)
Get number field coverage up to 100%
Reported by: | David Loeffler | Owned by: | David Loeffler |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6 |
Component: | number fields | Keywords: | doctest |
Cc: | Rishikesh, Jeroen Demeyer | Merged in: | sage-4.6.alpha2 |
Authors: | David Loeffler | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This patch does the following:
- Removes the outdated file sage.rings.number_field.totallyreal_dsage (DSage was removed months ago).
- ReSTifies several number field files and adds them to the reference manual.
- Adds doctests to 38 functions and removes 21 functions that are deprecated or duplicate, getting doctest coverage for number fields up to 100%.
Attachments (4)
Change History (32)
comment:1 Changed 12 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 12 years ago by
comment:3 Changed 12 years ago by
Do you thing that
# TODO -- relative extensions need to be completely rewritten, so one # can get easy access to representation of elements in their relative # form. Functions like matrix below can't be done until relative # extensions are re-written this way. Also there needs to be class # hierarchy for number field elements, integers, etc. This is a # nontrivial project, and it needs somebody to attack it. I'm amazed # how long this has gone unattacked. # Relative elements need to be a derived class or something. This is # terrible as it is now.
is no longer valid?
comment:4 Changed 12 years ago by
I confirm that I think that comment is no longer valid.
The comment has been lying undisturbed in the code for more than three years now (i.e. more than half of the entire lifespan of the Sage project), as "hg annotate" will show you. There were issues with matrix, getitem, etc for relative number field elements (e.g. #2551) but they have all been fixed now (a large number of them thanks to fwclarke's sterling work at #5508, which closed five or six tickets in one go).
comment:5 Changed 12 years ago by
Well, I think that it still applies the fact that relative number fields are slow. The construction of an underlying absolute number field to make computations has coefficient explosion problems. Also, there should be smarter calls to pari_nf pari_rnf that are operations that can be very costly.
So yes, the code works but it is inefficient in some interesting cases.
comment:6 Changed 12 years ago by
Cc: | Rishikesh added |
---|
comment:7 Changed 12 years ago by
Authors: | → David Loeffler |
---|
I've uploaded a new patch which works with 4.6.alpha1 (no massive changes, just a chunk that was rejected because it fixed a whitespace error also fixed by #8680, and two doctests that needed fixing because of the PARI update). Please, someone review this, before it bitrots again!
comment:8 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:9 Changed 12 years ago by
I still think that the comment about rewriting relative extensions still apply. But there have been changes, so I will think some alternative to that text.
comment:10 Changed 12 years ago by
Cc: | Jeroen Demeyer added |
---|
comment:11 Changed 12 years ago by
I fully agree that relative extensions are not optimally implemented. By all means open another trac ticket for that if you like; but the specific issues (with matrix, etc) that the comment refers to are fixed (see #2551).
comment:12 Changed 12 years ago by
I personally prefer to leave
sage: I.ideal_below() Fractional ideal (b) # 32-bit Fractional ideal (-b) # 64-bit
as it is, instead of
sage: I.ideal_below() == F.ideal(b) True
As an example, the first is more clear. But as I said, this is just a personal preference.
comment:13 Changed 12 years ago by
Changed 12 years ago by
Attachment: | trac_9359.patch added |
---|
New version without change to "ideal_below"
comment:14 Changed 12 years ago by
OK, I'm fine with that. I've just uploaded a patch with that hunk removed.
comment:15 Changed 12 years ago by
I think you should put
R.<x> = PolynomialRing(QQ)
or
x = polygen(QQ)
everywhere before
K.<foo> = NumberField(some_polynomial_in_x)
I imagine users copy-pasting the examples and finding they don't work because x
is something else.
comment:16 Changed 12 years ago by
In my defence, I'd like to point out that your comment applies to most of the 600 or so number field doctests that were already there -- not just the 38 that I added -- and nobody's complained about it before :-) So we could have another ticket if you think it's an issue, but I claim it's orthogonal to this ticket.
(I personally think having the doctests this way is a good thing, as it makes it clear that you can start Sage and type K.<a> = Number Field(x^2 - 37)
and it will Just Work (tm), unlike in Magma for instance.)
comment:17 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
Nice job, looks good to me!
comment:18 Changed 12 years ago by
Reviewers: | → Jeroen Demeyer |
---|
comment:19 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Status: | positive_review → needs_work |
There are a few conflicts with #8334. I will rebase this patch.
Changed 12 years ago by
Attachment: | 9359_rebase_to_8334.patch added |
---|
comment:20 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
With the attached patch, make ptestlong
succeeds on 32-bit and 64-bit systems.
comment:22 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
Looks fine -- thanks for cleaning that up.
comment:23 Changed 12 years ago by
Status: | positive_review → needs_work |
---|
Could someone update the commit string of the patch so its first line is a < 80 (or so) character summary of the changes that includes the ticket number, then restore the positive review? Additional lines are no problem, of course.
Changed 12 years ago by
Attachment: | trac_9359-fixed.patch added |
---|
Qfolded patch with better commit string. Apply only this patch.
comment:24 Changed 12 years ago by
Status: | needs_work → positive_review |
---|
Done. I also qfolded into one patch.
comment:25 Changed 12 years ago by
Status: | positive_review → needs_work |
---|
Thanks, David. I get some docbuild warnings
/mnt/usb1/scratch/mpatel/tmp/sage-4.6.alpha1-working/devel/sage/doc/en/reference/sage/rings/number_field/totallyreal.rst:71: WARNING: duplicate citation C, other instance in /mnt/usb1/scratch/mpatel/tmp/sage-4.6.alpha1-working/devel/sage/doc/en/reference/sage/rings/number_field/small_primes_of_degree_one.rst /mnt/usb1/scratch/mpatel/tmp/sage-4.6.alpha1-working/devel/sage/doc/en/reference/sage/rings/number_field/totallyreal.rst:75: WARNING: duplicate citation M, other instance in /mnt/usb1/scratch/mpatel/tmp/sage-4.6.alpha1-working/devel/sage/doc/en/reference/sage/crypto/lfsr.rst /mnt/usb1/scratch/mpatel/tmp/sage-4.6.alpha1-working/devel/sage/doc/en/reference/sage/rings/number_field/totallyreal.rst:82: WARNING: duplicate citation V, other instance in /mnt/usb1/scratch/mpatel/tmp/sage-4.6.alpha1-working/devel/sage/doc/en/reference/sage/matrix/matrix2.rst
with the current merger-4.6.alpha2 "applied" to 4.6.alpha1.
Could you or Jeroen add a docfix patch?
comment:26 follow-up: 28 Changed 12 years ago by
Status: | needs_work → positive_review |
---|
Docfix patch as requested. We should really have a single table of citations for the whole reference manual -- one of the conflicts here was with another file citing 'the same textbook' -- but this'll do for now.
comment:27 Changed 12 years ago by
Merged in: | → sage-4.6.alpha2 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:28 Changed 12 years ago by
Replying to davidloeffler:
Docfix patch as requested. We should really have a single table of citations for the whole reference manual -- one of the conflicts here was with another file citing 'the same textbook' -- but this'll do for now.
This sage-devel thread seems related.
Here's a patch; it depends on #9244 (but *not* on #9336).