Opened 7 months ago
Closed 6 months ago
#23809 closed defect (fixed)
Fix normalization for morphisms defined over QQbar in canonical_height
Reported by:  paulfili  Owned by:  

Priority:  major  Milestone:  sage8.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 (3x^{2, 3y}2), but when the resultant is computed with normalize=True, we are really computing the resultant of (x^{2,y}2), 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 7 months ago by
 Branch set to u/paulfili/pcffix
comment:2 Changed 7 months ago by
 Commit set to b1b1e34e979f9b341b48a2da75d86bc79a855266
comment:3 Changed 7 months ago by
 Dependencies changed from 23497 to #23497
 Status changed from new to needs_review
comment:4 Changed 7 months ago by
 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 6 months ago by
 Commit changed from b1b1e34e979f9b341b48a2da75d86bc79a855266 to 9d2cd0a91cfe23b5c1bd2deaab7faa611c6fe350
Branch pushed to git repo; I updated commit sha1. New commits:
9d2cd0a  Added handling for the case where Wells' algorithm returns a negative value

comment:6 Changed 6 months ago by
 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 6 months ago by
 Commit changed from 9d2cd0a91cfe23b5c1bd2deaab7faa611c6fe350 to 2ea6ff8f59494d84592e6025a34a6f5a5f559641
Branch pushed to git repo; I updated commit sha1. New commits:
2ea6ff8  Fixed doc tests to match new functionality.

comment:8 Changed 6 months ago by
Doc tests have been updated, this should be good to go.
comment:9 Changed 6 months ago by
 Branch changed from u/paulfili/pcffix to u/bhutz/pcffix
comment:10 Changed 6 months ago by
 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:
323f9e9  23809: added assert

comment:11 Changed 6 months ago by
 Status changed from needs_review to positive_review
comment:12 Changed 6 months ago by
 Commit changed from 323f9e9f63493a622971aa34acb1ac894ffa47a3 to 095fc5b836f269249130b1bb0b6003141a17c401
 Status changed from positive_review to needs_review
comment:14 Changed 6 months ago by
 Status changed from needs_work to needs_review
Paul, Just double check I didn't mess it up fixing the merge conflict
comment:16 Changed 6 months ago by
 Branch changed from u/bhutz/pcffix to 095fc5b836f269249130b1bb0b6003141a17c401
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Added a normalization needed when converting from QQbar