Opened 14 years ago

Closed 14 years ago

#5508 closed enhancement (fixed)

[with patch, positive review] Improvements for relative number fields

Reported by: Francis Clarke Owned by: William Stein
Priority: major Milestone: sage-3.4.1
Component: number theory Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

The attached patch implements many improvements for relative number fields. In particular a whole load of previously unimplemented functions for ideals in a relative number field now work, and others work better.

Following discussion at sage-nt thread, for several functions the distinction between the relative and absolute version has been made explicit, in order to avoid ambiguity. Thus, for example, for a relative number field both relative_degree and absolute_degree are defined but degree is unimplemented, while for an absolute number field relative_degree, absolute_degree and degree are all defined (with the same meaning). This has entailed a few minor changes to enable functions to work with either absolute or relative number fields.

It has been suggested that NumberField should only be allowed to generate an absolute number field. I have not implemented this, but I have made NumberFieldTower publicly available and used it in several doctests. If a change was made to NumberField, NumberFieldTower could retain the old functionality of NumberField.

A number of other minor changes have been made, and these seem to fix #5276, #5214 and #2551

Attachments (3)

sage-5508.patch (64.1 KB) - added by Francis Clarke 14 years ago.
sage-5508.2.patch (64.1 KB) - added by John Cremona 14 years ago.
replaces previous
sage-5508.3.patch (64.1 KB) - added by Francis Clarke 14 years ago.
Replaces previous

Download all attachments as: .zip

Change History (16)

Changed 14 years ago by Francis Clarke

Attachment: sage-5508.patch added

comment:1 Changed 14 years ago by John Cremona

Summary: [with patch, needs review] Improvements for relative number fields[with patch, positive review, one tiny fix needed] Improvements for relative number fields

Review: I read through the patch and was impressed by the thoroughness and attention to detail! I don't know all the formulas for relative different (etc) off the top of my head, but what is tere looks reasonable.

The patch applies cleanly to 3.4.

Doctesting sage/rings/number_field, the only problem was this:

sage -t  "local/sage-3.4/devel/sage-5508/sage/rings/number_field//order.py"
**********************************************************************
File "/home/masgaj/local/sage-3.4/devel/sage-5508/sage/rings/number_field/order.py", line 1196:
    sage: OK(a)
Expected nothing
Got:
    a
**********************************************************************
File "/home/masgaj/local/sage-3.4/devel/sage-5508/sage/rings/number_field/order.py", line 1197:
    sage: a
Expected nothing
Got:
    a

which is just a matter of deleting a rogue "sage: " prompt in front of one line of output.

Fix that and this will ready to go. (I hope it merges ok with my units code at #5513!)

comment:2 Changed 14 years ago by David Loeffler

This also seems to fix #4193.

Changed 14 years ago by John Cremona

Attachment: sage-5508.2.patch added

replaces previous

comment:3 Changed 14 years ago by John Cremona

Summary: [with patch, positive review, one tiny fix needed] Improvements for relative number fields[with patch, with positive review] Improvements for relative number fields

I can confirm that #4193, #5276, #5214 and #2551 can all be closed as fixed once this one is merged.

I have attached a patch whic his identical to the original except that it ocrrects one typo which means that all doctests in sage/rings/number_fields pass, and hence this one gets a (very!) positive review.

comment:4 Changed 14 years ago by Francis Clarke

Summary: [with patch, with positive review] Improvements for relative number fields[with patch, more work needed] Improvements for relative number fields

The patch doesn't actually merge well with the #5513 patches. Moreover, after some intensive testing, I have found a whole lot more minor changes needed to give relative number fields most of the functionality of their absolute versions. So my intention is to incorporate these a new patch which will merge ok.

comment:5 Changed 14 years ago by David Loeffler

Hmmm -- would you mind leaving this one as it is, since it's already been given a positive review, and making a new ticket for the follow-up patch, which can then be reviewed separately? I spent the best part of a day's work rebasing my patch at #5159 so it would apply happily on top of sage-5508.2.patch!

(One wouldn't normally try and make improvements to a journal paper after it's been accepted for publication; one writes follow-up papers.)

comment:6 Changed 14 years ago by David Loeffler

Summary: [with patch, more work needed] Improvements for relative number fields[with patch, positive review] Improvements for relative number fields

comment:7 Changed 14 years ago by Francis Clarke

Yes, fine, let's leave it as it is. I'll do the new changes separately, but not for a few days.

comment:8 Changed 14 years ago by Michael Abshoff

Summary: [with patch, positive review] Improvements for relative number fields[with patch, needs work] Improvements for relative number fields

Does this patch depend on any other patch set? I am seeing the following doctest failure:

sage-3.4.1.alpha0$ ./sage -t -long devel/sage/sage/rings/number_field/number_field_ideal_rel.py
sage -t -long "devel/sage/sage/rings/number_field/number_field_ideal_rel.py"
**********************************************************************
File "/scratch/mabshoff/sage-3.4.1.alpha0/devel/sage/sage/rings/number_field/number_field_ideal_rel.py", line 598:
    sage: z = I.element_1_mod(J); z
Expected:
    -21/2*b*a - 21/2
Got:
    -8*b*a + 24

Cheers,

Michael

comment:9 Changed 14 years ago by Francis Clarke

When I built it on top of 3.4 I got -21/2*b*a - 21/2, so I think something else must be the cause. Of course -8*b*a + 24 is also an acceptable answer.

comment:10 in reply to:  9 ; Changed 14 years ago by Michael Abshoff

Replying to fwclarke:

When I built it on top of 3.4 I got -21/2*b*a - 21/2, so I think something else must be the cause. Of course -8*b*a + 24 is also an acceptable answer.

Ok. Can you change the doctest since some other patches depend on this patch?

Cheers,

Michael

comment:11 in reply to:  10 Changed 14 years ago by Francis Clarke

Replying to mabshoff:

Ok. Can you change the doctest since some other patches depend on this patch?

The new sage-5508.3.patch has the revised doctest, but is otherwise the same as sage-5508.2.patch

Hope this solves the problem.

Francis

Changed 14 years ago by Francis Clarke

Attachment: sage-5508.3.patch added

Replaces previous

comment:12 Changed 14 years ago by Michael Abshoff

Summary: [with patch, needs work] Improvements for relative number fields[with patch, positive review] Improvements for relative number fields

Ok, back to positive review then :)

Cheers,

Michael

comment:13 Changed 14 years ago by Michael Abshoff

Resolution: fixed
Status: newclosed

Merged sage-5508.3.patch in Sage 3.4.1.alpha0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.