Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#17082 closed enhancement (fixed)

Fix height_difference_bound to work over Number Fields

Reported by: jdefaria Owned by:
Priority: minor Milestone: sage-6.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 jdefaria

  • 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 git

  • Commit set to f64e556c31802d06134265b225cc3f6cb715bf28

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

comment:3 Changed 3 years ago by jdefaria

  • Status changed from new to needs_review

comment:4 Changed 3 years ago by bhutz

  • 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 git

  • Commit changed from f64e556c31802d06134265b225cc3f6cb715bf28 to 98959fb6c4ae60350aeb171f3740483c8c56dc30

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

98959fb17082 - fixed error checks

comment:6 Changed 3 years ago by git

  • Commit changed from 98959fb6c4ae60350aeb171f3740483c8c56dc30 to 09349098e9a50e01761b46402198c40ba3becd3c

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

093490917082- added num field example

comment:7 Changed 3 years ago by jdefaria

  • Status changed from needs_work to needs_review

comment:8 Changed 3 years ago by bhutz

  • 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 git

  • Commit changed from 09349098e9a50e01761b46402198c40ba3becd3c to 4102df29b08838365ab6719f0cb04b7ac43a5bc0

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

4102df2Changed from ZZ to number field rings

comment:10 Changed 3 years ago by jdefaria

  • 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 git

  • Commit changed from 4102df29b08838365ab6719f0cb04b7ac43a5bc0 to 68978c27de6990e4c9f8010a4447264c09aea871

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

68978c2Fixed small whitespace issues and condensed code

comment:12 Changed 3 years ago by git

  • Commit changed from 68978c27de6990e4c9f8010a4447264c09aea871 to 0f9a1f375619114cb9232f9c49dfaacafc03c44e

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

0f9a1f3More Typos and whitespace errors

comment:13 Changed 3 years ago by bhutz

  • Status changed from needs_review to positive_review

comment:14 Changed 3 years ago by vbraun

  • 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 jdemeyer

  • Authors changed from Joao de Faria to Joao Alberto de Faria
  • Commit 0f9a1f375619114cb9232f9c49dfaacafc03c44e deleted
Note: See TracTickets for help on using tickets.