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

Fix height_difference_bound to work over Number Fields #17082

Closed
sagetrac-jdefaria mannequin opened this issue Oct 1, 2014 · 20 comments
Closed

Fix height_difference_bound to work over Number Fields #17082

sagetrac-jdefaria mannequin opened this issue Oct 1, 2014 · 20 comments

Comments

@sagetrac-jdefaria
Copy link
Mannequin

sagetrac-jdefaria mannequin commented Oct 1, 2014

After checking the math, saw that there is no reason to restrict to QQ or ZZ only, fixing checks at start to reflect this.

Component: algebraic geometry

Author: Joao Alberto de Faria

Branch: 0f9a1f3

Reviewer: Ben Hutz

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

@sagetrac-jdefaria sagetrac-jdefaria mannequin added this to the sage-6.4 milestone Oct 1, 2014
@sagetrac-jdefaria
Copy link
Mannequin Author

sagetrac-jdefaria mannequin commented Oct 16, 2014

Branch: u/jdefaria/ticket/17082

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2014

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

a524288Adapted code to not have to check for numerator
f64e556Deleted lines used for testing

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2014

Commit: f64e556

@bhutz
Copy link

bhutz commented Nov 13, 2014

comment:4

This fails doc tests ('Not Implemented Error' spelled wrong) and needs a numberfield example

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 3, 2014

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

98959fb17082 - fixed error checks

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 3, 2014

Changed commit from f64e556 to 98959fb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 3, 2014

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

093490917082- added num field example

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 3, 2014

Changed commit from 98959fb to 0934909

@bhutz
Copy link

bhutz commented Dec 15, 2014

comment:8

Well, a couple things here. This passes all the long tests, and the functionality change seems ok. However, there are two things that should be fixed.

  • There is an indentation issue with the new example you added.

  • you seems to restrict to number fields or ZZ. If it is ZZ the code changes the ring to QQ so that .lift() works. Why can't you do this for any number field ring (change to its field of fractions)?

@bhutz
Copy link

bhutz commented Dec 15, 2014

Reviewer: Ben Hutz

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2015

Changed commit from 0934909 to 4102df2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2015

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

4102df2Changed from ZZ to number field rings

@sagetrac-jdefaria
Copy link
Mannequin Author

sagetrac-jdefaria mannequin commented Jan 15, 2015

comment:10

I opened the documentation, didn't see any problems there, everything else is fixed, needs review

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 16, 2015

Changed commit from 4102df2 to 68978c2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 16, 2015

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

68978c2Fixed small whitespace issues and condensed code

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 16, 2015

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

0f9a1f3More Typos and whitespace errors

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 16, 2015

Changed commit from 68978c2 to 0f9a1f3

@vbraun
Copy link
Member

vbraun commented Jan 24, 2015

Changed branch from u/jdefaria/ticket/17082 to 0f9a1f3

@jdemeyer
Copy link

Changed commit from 0f9a1f3 to none

@jdemeyer
Copy link

Changed author from Joao de Faria to Joao Alberto de Faria

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

3 participants