Opened 8 years ago

Closed 6 years ago

#12183 closed enhancement (fixed)

absolute and relative norm functions for number field elements

Reported by: MvanBeek Owned by: davidloeffler
Priority: major Milestone: sage-5.12
Component: number fields Keywords:
Cc: Merged in: sage-5.12.beta4
Authors: Monique van Beek Reviewers: Michiel Kosters, Angelos Koutsianas
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by davidloeffler)

Although number_field_element.pyx has a norm method, it does not have absolute_norm() and relative_norm(). This would be a useful shorthand to have.

Apply 12183_5.patch.

Attachments (4)

12183.patch (3.6 KB) - added by MvanBeek 8 years ago.
first version of the patch
12183.2.patch (2.8 KB) - added by MvanBeek 8 years ago.
second version, this time without erroneous extra stuff from another patch
12183.3.patch (2.9 KB) - added by MvanBeek 8 years ago.
whitespace conventions followed, also an extra test added in relative_norm tests in number_field_element
12183_5.patch (2.8 KB) - added by mkosters 6 years ago.

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by MvanBeek

first version of the patch

Changed 8 years ago by MvanBeek

second version, this time without erroneous extra stuff from another patch

Changed 8 years ago by MvanBeek

whitespace conventions followed, also an extra test added in relative_norm tests in number_field_element

comment:1 Changed 8 years ago by mkosters

  • Component changed from PLEASE CHANGE to number fields
  • Owner changed from tbd to davidloeffler
  • Priority changed from trivial to major
  • Reviewers set to Michiel Kosters
  • Type changed from PLEASE CHANGE to enhancement

I've applied the patch and did all the tests for rings (all passed). I've also done some computation of my own. I've looked at the code and it looks fine. I've built the documentation and it looks fine.

comment:2 Changed 8 years ago by mkosters

  • Status changed from new to needs_review

comment:3 Changed 8 years ago by mkosters

  • Status changed from needs_review to positive_review

comment:4 Changed 8 years ago by johanbosman

  • Description modified (diff)

comment:5 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Monique, you should put a proper commit message to your patch. If you use Mercurial queues (which you do), this can be done using

hg qrefresh -e

and then

hg export tip

as usual.

Changed 6 years ago by mkosters

comment:6 Changed 6 years ago by mkosters

Add 12183_5.patch without any of the previous patches. Made the code of Monique a bit shorter and added a commit.

comment:7 Changed 6 years ago by mkosters

  • Status changed from needs_work to needs_review

comment:8 Changed 6 years ago by akoutsianas

  • Status changed from needs_review to positive_review

The patch works for the version 5.11.beta3.

comment:9 Changed 6 years ago by akoutsianas

  • Reviewers changed from Michiel Kosters to Michiel Kosters, Angelos Koutsianas

comment:10 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:11 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

The patch needs a proper commit message (use hg qrefresh -e for that).

comment:12 Changed 6 years ago by jdemeyer

The patch should also be rebased to sage-5.12.beta2.

comment:13 Changed 6 years ago by davidloeffler

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

The patch doesn't actually need rebasing. The problem here is that Michiel made a new patch 12183_5.patch, and Angelos gave a positive review to that patch, but the ticket description still said "apply 12183.3.patch". I have corrected the ticket description, and verified that the latest patch does have a proper commit message and applies without fuzz to the latest 5.12.beta2.

comment:14 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.12.beta4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.