Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute Hasse invariant over number fields and fix current implementation #16379

Closed
annahaensch mannequin opened this issue May 20, 2014 · 13 comments
Closed

Compute Hasse invariant over number fields and fix current implementation #16379

annahaensch mannequin opened this issue May 20, 2014 · 13 comments

Comments

@annahaensch
Copy link
Mannequin

annahaensch mannequin commented May 20, 2014

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.

Component: quadratic forms

Author: Anna Haensch

Branch/Commit: f8fec47

Reviewer: Peter Bruin

Issue created by migration from https://trac.sagemath.org/ticket/16379

@annahaensch annahaensch mannequin added this to the sage-6.3 milestone May 20, 2014
@annahaensch
Copy link
Mannequin Author

annahaensch mannequin commented May 20, 2014

Branch: u/annahaensch/ticket/16379

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 21, 2014

Commit: 471f157

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 21, 2014

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

471f157updated examples from last commit

@annahaensch annahaensch mannequin added the s: needs review label May 21, 2014
@pjbruin
Copy link
Contributor

pjbruin commented May 23, 2014

comment:4

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

@pjbruin
Copy link
Contributor

pjbruin commented May 23, 2014

Reviewer: Peter Bruin

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 23, 2014

Changed commit from 471f157 to a6fc438

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 23, 2014

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

a6fc438Removed trailing whitespace from last commit

@pjbruin
Copy link
Contributor

pjbruin commented May 23, 2014

comment:6

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 23, 2014

Changed commit from a6fc438 to fb0d360

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 23, 2014

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

fb0d360Reverted one example in doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 23, 2014

Changed commit from fb0d360 to f8fec47

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 23, 2014

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

f8fec47Reverts one doctest to old example, ignore last commit

@vbraun
Copy link
Member

vbraun commented May 25, 2014

Changed branch from u/annahaensch/ticket/16379 to f8fec47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants