#17082 closed enhancement (fixed)
Fix height_difference_bound to work over Number Fields
Reported by:  jdefaria  Owned by:  

Priority:  minor  Milestone:  sage6.4 
Component:  algebraic geometry  Keywords:  
Cc:  Merged in:  
Authors:  Joao Alberto de Faria  Reviewers:  Ben Hutz 
Report Upstream:  N/A  Work issues:  
Branch:  0f9a1f3 (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description
After checking the math, saw that there is no reason to restrict to QQ or ZZ only, fixing checks at start to reflect this.
Change History (15)
comment:1 Changed 3 years ago by
 Branch set to u/jdefaria/ticket/17082
 Created changed from 10/01/14 17:29:07 to 10/01/14 17:29:07
 Modified changed from 10/01/14 17:29:07 to 10/01/14 17:29:07
comment:2 Changed 3 years ago by
 Commit set to f64e556c31802d06134265b225cc3f6cb715bf28
comment:3 Changed 3 years ago by
 Status changed from new to needs_review
comment:4 Changed 3 years ago by
 Status changed from needs_review to needs_work
This fails doc tests ('Not Implemented Error' spelled wrong) and needs a numberfield example
comment:5 Changed 3 years ago by
 Commit changed from f64e556c31802d06134265b225cc3f6cb715bf28 to 98959fb6c4ae60350aeb171f3740483c8c56dc30
Branch pushed to git repo; I updated commit sha1. New commits:
98959fb  17082  fixed error checks

comment:6 Changed 3 years ago by
 Commit changed from 98959fb6c4ae60350aeb171f3740483c8c56dc30 to 09349098e9a50e01761b46402198c40ba3becd3c
Branch pushed to git repo; I updated commit sha1. New commits:
0934909  17082 added num field example

comment:7 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:8 Changed 3 years ago by
 Reviewers set to Ben Hutz
 Status changed from needs_review to needs_work
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)?
comment:9 Changed 3 years ago by
 Commit changed from 09349098e9a50e01761b46402198c40ba3becd3c to 4102df29b08838365ab6719f0cb04b7ac43a5bc0
Branch pushed to git repo; I updated commit sha1. New commits:
4102df2  Changed from ZZ to number field rings

comment:10 Changed 3 years ago by
 Status changed from needs_work to needs_review
I opened the documentation, didn't see any problems there, everything else is fixed, needs review
comment:11 Changed 3 years ago by
 Commit changed from 4102df29b08838365ab6719f0cb04b7ac43a5bc0 to 68978c27de6990e4c9f8010a4447264c09aea871
Branch pushed to git repo; I updated commit sha1. New commits:
68978c2  Fixed small whitespace issues and condensed code

comment:12 Changed 3 years ago by
 Commit changed from 68978c27de6990e4c9f8010a4447264c09aea871 to 0f9a1f375619114cb9232f9c49dfaacafc03c44e
Branch pushed to git repo; I updated commit sha1. New commits:
0f9a1f3  More Typos and whitespace errors

comment:13 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:14 Changed 3 years ago by
 Branch changed from u/jdefaria/ticket/17082 to 0f9a1f375619114cb9232f9c49dfaacafc03c44e
 Resolution set to fixed
 Status changed from positive_review to closed
comment:15 Changed 3 years ago by
 Commit 0f9a1f375619114cb9232f9c49dfaacafc03c44e deleted
Branch pushed to git repo; I updated commit sha1. New commits:
Adapted code to not have to check for numerator
Deleted lines used for testing