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: |
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)
Change History (16)
Changed 14 years ago by
Attachment: | sage-5508.patch added |
---|
comment:1 Changed 14 years ago by
Summary: | [with patch, needs review] Improvements for relative number fields → [with patch, positive review, one tiny fix needed] Improvements for relative number fields |
---|
comment:3 Changed 14 years ago by
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
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
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
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
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
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 follow-up: 10 Changed 14 years ago by
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 follow-up: 11 Changed 14 years ago by
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 Changed 14 years ago by
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
comment:12 Changed 14 years ago by
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
Resolution: | → fixed |
---|---|
Status: | new → closed |
Merged sage-5508.3.patch in Sage 3.4.1.alpha0.
Cheers,
Michael
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:
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!)