#9359 closed enhancement (fixed)
Get number field coverage up to 100%
Reported by: | davidloeffler | Owned by: | davidloeffler |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6 |
Component: | number fields | Keywords: | doctest |
Cc: | rishi, jdemeyer | 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 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
comment:3 Changed 10 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 10 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 10 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 10 years ago by
- Cc rishi added
comment:7 Changed 10 years ago by
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 10 years ago by
- Description modified (diff)
comment:9 Changed 10 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 10 years ago by
- Cc jdemeyer added
comment:11 Changed 10 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 10 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 10 years ago by
comment:14 Changed 10 years ago by
OK, I'm fine with that. I've just uploaded a patch with that hunk removed.
comment:15 Changed 10 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 10 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 10 years ago by
- Status changed from needs_review to positive_review
Nice job, looks good to me!
comment:18 Changed 10 years ago by
- Reviewers set to Jeroen Demeyer
comment:19 Changed 10 years ago by
- Description modified (diff)
- Status changed from positive_review to needs_work
There are a few conflicts with #8334. I will rebase this patch.
comment:20 Changed 10 years ago by
- Status changed from needs_work to needs_review
With the attached patch, make ptestlong
succeeds on 32-bit and 64-bit systems.
comment:21 Changed 10 years ago by
David, can you review my additions?
comment:22 Changed 10 years ago by
- Status changed from needs_review to positive_review
Looks fine -- thanks for cleaning that up.
comment:23 Changed 10 years ago by
- Status changed from positive_review to 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.
comment:24 Changed 10 years ago by
- Status changed from needs_work to positive_review
Done. I also qfolded into one patch.
comment:25 Changed 10 years ago by
- Status changed from positive_review to 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 10 years ago by
- Status changed from needs_work to 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 10 years ago by
- Merged in set to sage-4.6.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
comment:28 in reply to: ↑ 26 Changed 10 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).