Opened 8 years ago

Closed 8 years ago

#16379 closed enhancement (fixed)

Compute Hasse invariant over number fields and fix current implementation

Reported by: annahaensch Owned by:
Priority: minor Milestone: sage-6.3
Component: quadratic forms Keywords:
Cc: Merged in:
Authors: Anna Haensch Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: f8fec47 (Commits, GitHub, GitLab) Commit: f8fec470e7514f2ad4d8400da4006f63a1d9a2ff
Dependencies: Stopgaps:

Status badges

Description

This patch will enhance the current functions hasse_invariant and hasse_invariant_OMeara to include computations of Hasse invariant for quadratic forms over number fields. In addition, this patch fixes an indexing error in the original hasse_invariant_OMeara code, which currently reads

for j in range(n-1):
    for k in range(j, n):

but should read

for j in range(n):
    for k in range(j, n):

to be consistent with OMeara's algorithm.

Change History (10)

comment:1 Changed 8 years ago by annahaensch

  • Branch set to u/annahaensch/ticket/16379
  • Created changed from 05/20/14 09:25:46 to 05/20/14 09:25:46
  • Modified changed from 05/20/14 09:25:46 to 05/20/14 09:25:46

comment:2 Changed 8 years ago by git

  • Commit set to 471f15734adb60b20b0bc3d6c37aeaa5c2c2e5f5

Branch pushed to git repo; I updated commit sha1. New commits:

471f157updated examples from last commit

comment:3 Changed 8 years ago by annahaensch

  • Status changed from new to needs_review

comment:4 follow-up: Changed 8 years ago by pbruin

  • Reviewers set to Peter Bruin

Looks good to me except for two minor points:

  • it is best to keep existing doctests as far as possible; only add new ones, and fix old ones if necessary instead of replacing them;
  • your patch introduces trailing whitespace on several lines (use e.g. git diff --color develop...YOURBRANCH to see this); this is discouraged, so could you remove it if and when you make another commit?

(The patchbot encountered a doctest failure on 6.3.beta1, but it seems to be unrelated to your patch.)

comment:5 Changed 8 years ago by git

  • Commit changed from 471f15734adb60b20b0bc3d6c37aeaa5c2c2e5f5 to a6fc438f5ec644581e6ba962bf9cab1a4a4a6253

Branch pushed to git repo; I updated commit sha1. New commits:

a6fc438Removed trailing whitespace from last commit

comment:6 in reply to: ↑ 4 Changed 8 years ago by pbruin

Coming back to this point:

  • it is best to keep existing doctests as far as possible; only add new ones, and fix old ones if necessary instead of replacing them;

I now see that there were two identical doctests (with DiagonalQuadraticForm(ZZ, [1, -1, -1])) and you changed [1, -1, -1] to [1, -1, 5] in both of them. Could you change one of them back to [1, -1, -1]? Then we get both your new doctest and the old one, instead of duplicates.

comment:7 Changed 8 years ago by git

  • Commit changed from a6fc438f5ec644581e6ba962bf9cab1a4a4a6253 to fb0d36017bc6118efe57935885464790b94c9d23

Branch pushed to git repo; I updated commit sha1. New commits:

fb0d360Reverted one example in doctest

comment:8 Changed 8 years ago by git

  • Commit changed from fb0d36017bc6118efe57935885464790b94c9d23 to f8fec470e7514f2ad4d8400da4006f63a1d9a2ff

Branch pushed to git repo; I updated commit sha1. New commits:

f8fec47Reverts one doctest to old example, ignore last commit

comment:9 Changed 8 years ago by pbruin

  • Status changed from needs_review to positive_review

comment:10 Changed 8 years ago by vbraun

  • Branch changed from u/annahaensch/ticket/16379 to f8fec470e7514f2ad4d8400da4006f63a1d9a2ff
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.