Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

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

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%.

Dependencies: #7883, #9898, #9753, #9764, #8334

Attachments (4)

trac_9359.patch (97.3 KB) - added by davidloeffler 10 years ago.
New version without change to "ideal_below"
9359_rebase_to_8334.patch (3.5 KB) - added by jdemeyer 10 years ago.
Apply on top of #7883, #9898, #9753, #9764, #8334 and trac_9359.patch
trac_9359-fixed.patch (97.8 KB) - added by davidloeffler 10 years ago.
Qfolded patch with better commit string. Apply only this patch.
trac_9359-docfix.patch (2.4 KB) - added by davidloeffler 10 years ago.
fix duplicate citation

Download all attachments as: .zip

Change History (32)

comment:1 Changed 10 years ago by davidloeffler

  • Status changed from new to needs_review

Here's a patch; it depends on #9244 (but *not* on #9336).

comment:2 Changed 10 years ago by davidloeffler

I've now moved one doctest from here to #9244, with the result that this patch is now independent of #9244 (and #9336). It does require the (positively-reviewed) patch at #9242. (Without #9242 it applies but one TestSuite doctest fails.)

comment:3 Changed 10 years ago by lftabera

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 davidloeffler

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 lftabera

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 rishi

  • Cc rishi added

comment:7 Changed 10 years ago by davidloeffler

  • Authors set to 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 10 years ago by davidloeffler

  • Description modified (diff)

comment:9 Changed 10 years ago by lftabera

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 jdemeyer

  • Cc jdemeyer added

comment:11 Changed 10 years ago by davidloeffler

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 jdemeyer

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 jdemeyer

Also, the hunk

-            sage: I.ideal_below()
-            Fractional ideal (-b)  # 32-bit
-            Fractional ideal (b)   # 64-bit
+            sage: I.ideal_below() == F.ideal(b)
+            True

conflicts with #9764. So would you mind removing it?

Apart from that, the patch applies fine and doctests fine, even with #9753 and #9764 applied.

Changed 10 years ago by davidloeffler

New version without change to "ideal_below"

comment:14 Changed 10 years ago by davidloeffler

OK, I'm fine with that. I've just uploaded a patch with that hunk removed.

comment:15 Changed 10 years ago by jdemeyer

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 davidloeffler

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 jdemeyer

  • Status changed from needs_review to positive_review

Nice job, looks good to me!

comment:18 Changed 10 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

comment:19 Changed 10 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from positive_review to needs_work

There are a few conflicts with #8334. I will rebase this patch.

Changed 10 years ago by jdemeyer

Apply on top of #7883, #9898, #9753, #9764, #8334 and trac_9359.patch

comment:20 Changed 10 years ago by jdemeyer

  • 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 jdemeyer

David, can you review my additions?

comment:22 Changed 10 years ago by davidloeffler

  • Status changed from needs_review to positive_review

Looks fine -- thanks for cleaning that up.

comment:23 Changed 10 years ago by mpatel

  • 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.

Changed 10 years ago by davidloeffler

Qfolded patch with better commit string. Apply only this patch.

comment:24 Changed 10 years ago by davidloeffler

  • Status changed from needs_work to positive_review

Done. I also qfolded into one patch.

comment:25 Changed 10 years ago by mpatel

  • 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?

Changed 10 years ago by davidloeffler

fix duplicate citation

comment:26 follow-up: Changed 10 years ago by davidloeffler

  • 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 mpatel

  • 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 mpatel

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.

Note: See TracTickets for help on using tickets.