#23809 closed defect (fixed)

Fix normalization for morphisms defined over QQbar in canonical_height

Reported by: paulfili Owned by:
Priority: major Milestone: sage-8.1
Component: dynamics Keywords: canonical height
Cc: bhutz Merged in:
Authors: Paul Fili Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: 095fc5b (Commits) Commit: 095fc5b836f269249130b1bb0b6003141a17c401
Dependencies: #23497 Stopgaps:

Description

Given a projective dynamical system defined over QQbar, when we compute the canonical height of a point, we first find a number field where the point and function are both defined. Unfortunately while the resultant of this map was normalized, its coordinates were not necessarily normalized. This resulted in errors like this:

m=3
K.<v>=CyclotomicField(m)
P.<x,y>=ProjectiveSpace(K,1)
H=End(P)
f=DynamicalSystem_projective([3*y^2,-3*x^2])
f.is_post_critically_finite() # returns true 
f.change_ring(QQbar)
f.is_post_critically_finite() # returns false

The reason is that when the function is defined over K, and then changed to QQbar, it remains in the form (3x2, -3y2), but when the resultant is computed with normalize=True, we are really computing the resultant of (x2,-y2), which is what normalize_coordinates on f should have returned, but did not. As a result, when computing the local height of the critical points:

P, Q = f.critical_points()
f.canonical_height(P) # returns log(3) = 1.0642806546472312635391438233
#when it should be 0

We fix this behavior by normalizing the coordinates of f after defining it over the appropriate number field.

We also optimize a bit by checking if Wells' algorithm over QQ can be used *after* converting from QQbar to a number field, rather than *before* as it used to do.

Change History (16)

comment:1 Changed 11 months ago by paulfili

  • Branch set to u/paulfili/pcf-fix

comment:2 Changed 11 months ago by git

  • Commit set to b1b1e34e979f9b341b48a2da75d86bc79a855266

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

b1b1e34Added a normalization needed when converting from QQbar

comment:3 Changed 11 months ago by paulfili

  • Dependencies changed from 23497 to #23497
  • Status changed from new to needs_review

comment:4 Changed 11 months ago by bhutz

  • Reviewers set to Ben Hutz
  • Status changed from needs_review to needs_work

Just one thing here. I think returning a negative value is not good. If the negative value is within error bound of 0, I'd be tempted to return 0. If the negative value is beyond the error, then some kind of error should be raised as something went very wrong.

comment:5 Changed 11 months ago by git

  • Commit changed from b1b1e34e979f9b341b48a2da75d86bc79a855266 to 9d2cd0a91cfe23b5c1bd2deaab7faa611c6fe350

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

9d2cd0aAdded handling for the case where Wells' algorithm returns a negative value

comment:6 Changed 11 months ago by paulfili

  • Status changed from needs_work to needs_review

Thanks for the suggestion, Ben! I have included code to handle the case of Wells' algorithm returning a negative value now. (It does a sanity check that the negative is within the error from 0 as well and raises a ValueError? in the hopefully impossible case of it begin less than -error_bound.)

comment:7 Changed 11 months ago by git

  • Commit changed from 9d2cd0a91cfe23b5c1bd2deaab7faa611c6fe350 to 2ea6ff8f59494d84592e6025a34a6f5a5f559641

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

2ea6ff8Fixed doc tests to match new functionality.

comment:8 Changed 11 months ago by paulfili

Doc tests have been updated, this should be good to go.

comment:9 Changed 11 months ago by bhutz

  • Branch changed from u/paulfili/pcf-fix to u/bhutz/pcf-fix

comment:10 Changed 11 months ago by bhutz

  • Authors changed from paulfili to Paul Fili
  • Commit changed from 2ea6ff8f59494d84592e6025a34a6f5a5f559641 to 323f9e9f63493a622971aa34acb1ac894ffa47a3

This looks fine to me except I think an assertion is better. If this is fine with you go ahead and mark it positive.


New commits:

323f9e923809: added assert

comment:11 Changed 11 months ago by paulfili

  • Status changed from needs_review to positive_review

comment:12 Changed 11 months ago by git

  • Commit changed from 323f9e9f63493a622971aa34acb1ac894ffa47a3 to 095fc5b836f269249130b1bb0b6003141a17c401
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

2eecc68Merge branch 8.1.beta5 into t/23497/arith_dyn-23497
095fc5bMerge branch 't/23497/arith_dyn-23497' into t/23809/pcf-fix

comment:13 Changed 11 months ago by bhutz

  • Status changed from needs_review to needs_work

merge conflict

comment:14 Changed 11 months ago by bhutz

  • Status changed from needs_work to needs_review

Paul, Just double check I didn't mess it up fixing the merge conflict

comment:15 Changed 11 months ago by paulfili

  • Status changed from needs_review to positive_review

Looks good!

comment:16 Changed 11 months ago by vbraun

  • Branch changed from u/bhutz/pcf-fix to 095fc5b836f269249130b1bb0b6003141a17c401
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.