Implement is_newton for dynamical systems

3f66d54a-aea2-4c5e-a49e-4b2f63716fcf commented 5 years ago

We want to implement a function that checks whether a dynamical system is conjugate to a map of the form f(z) = z - p(z)/p'(z) where p is a square free polynomial.

Component: dynamics

Keywords: SI2019, sd104

Author: Anna Chlopecki, Simon Xu, Juliano Levier-Gomes, Grayson Jorgenson

Branch: da59247

Reviewer: Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley, Travis Scrimshaw, Frédéric Chapoton

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

3888886c-5df1-4374-9dcd-7276384261ce commented 5 years ago

Branch: u/gh-annanc2/test

3f66d54a-aea2-4c5e-a49e-4b2f63716fcf commented 5 years ago

Commit: 408c3ad

3f66d54a-aea2-4c5e-a49e-4b2f63716fcf commented 5 years ago

New commits:

408c3ad 28173 : Added is_Newton function.

4edfbb98-6b40-4472-8655-ea92584e13fe commented 5 years ago

comment:3

The NotImplementedError ("Degree one Newton maps are trivial.") should not start with a capital letter and shouldn't end with punctuation
Small note: tuple([Npoly.derivative(z) == (z - N_aff[0]).denominator(), M]) is much more clear as Npoly.derivative(z) == (z - N_aff[0]).denominator(), M
This try except:

try:
    Fbar = self.change_ring(QQbar)
except ValueError:
    Fbar = self.change_ring(self.base_ring().embeddings(QQbar)[0])

is redundant. self.change_ring(QQbar) will pick an embedding if one isn't provided, not raise a ValueError (see is_postcritically_finite() for an example of this behavior)

This function should probably check to make sure self is a map from P1 to P1. Right now, sigma_1 = self.sigma_invariants(1) raises a NotImplementedError, but if that changes in the future, this function will break unexpectedly somewhere
One other very small thing: .is_Newton(return_conjugation=True) returns either the boolean false, or a tuple of (true, matrix). Maybe it should return (false, None) on failure, so that unwrapping (like is_n, mat = ....is_Newton(return_conjugation=True)) still works (see is_periodic(return_period=True) for an example of a similar situation)

A more mathematical problem: Why does this function go to field extensions to find conjugations? e.g.:

A.<z> = AffineSpace(QQ, 1)
f = DynamicalSystem_affine([z - (z^3 + 2*z)/(3*z^2 + 2)])
M = Matrix([[1, 2], [2, 1]])
F = f.homogenize(1).conjugate(M)

bl, mat = F.is_Newton(return_conjugation=True)
print mat
print mat.parent()
# gives:
[-1/3*a    2/3]
[ 2/3*a   -1/3]

Full MatrixSpace of 2 by 2 dense matrices over Number Field in a with defining polynomial y^2 + 2

Shouldn't it be able to find the inverse of the conjugation by M, in this example?

4edfbb98-6b40-4472-8655-ea92584e13fe commented 5 years ago

Reviewer: Talia Blum, Matt Torrence

39bf98b2-f392-42d4-a7cb-688d50feb567 commented 5 years ago

Changed branch from u/gh-annanc2/test to u/gh-ksldr/28173_newton

39bf98b2-f392-42d4-a7cb-688d50feb567 commented 5 years ago

New commits:

`ed362ff`	`Merge branch 'u/gh-annanc2/test' of git://trac.sagemath.org/sage into newton_change`
`c42c414`	`28173: fixed changes from review`

39bf98b2-f392-42d4-a7cb-688d50feb567 commented 5 years ago

Changed commit from 408c3ad to c42c414

3aaa1050-12f2-43fa-80a1-4bf81726958a commented 5 years ago

Changed branch from u/gh-ksldr/28173_newton to u/gh-HTalbott/28173_newton

4edfbb98-6b40-4472-8655-ea92584e13fe commented 5 years ago

New commits:

`7d8e7ca`	`Merge branch 'u/gh-ksldr/28173_newton' of git://trac.sagemath.org/sage into test`
`1c55249`	`28173: changed return type to tuple`

4edfbb98-6b40-4472-8655-ea92584e13fe commented 5 years ago

Changed branch from u/gh-HTalbott/28173_newton to u/gh-HTalbott/28173_newton_modified

4edfbb98-6b40-4472-8655-ea92584e13fe commented 5 years ago

Changed commit from c42c414 to 1c55249

4edfbb98-6b40-4472-8655-ea92584e13fe commented 5 years ago

comment:7

The commit 1c55249 changes the return type as we talked about. How does this look?

3aaa1050-12f2-43fa-80a1-4bf81726958a commented 5 years ago

Changed reviewer from Talia Blum, Matt Torrence to Talia Blum, Matt Torrence, Henry Talbott

f4f2417a-d7eb-4b3a-be0a-8fc882f53e00 commented 5 years ago

comment:9

This looks good to me, thanks!

67c74138-1ca1-45a7-bae8-3eb17fc35cc7 commented 4 years ago

Changed branch from u/gh-HTalbott/28173_newton_modified to u/atowsley/28173_newton_modified

67c74138-1ca1-45a7-bae8-3eb17fc35cc7 commented 4 years ago

Changed commit from 1c55249 to cfcd6fe

67c74138-1ca1-45a7-bae8-3eb17fc35cc7 commented 4 years ago

Changed reviewer from Talia Blum, Matt Torrence, Henry Talbott to Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley

67c74138-1ca1-45a7-bae8-3eb17fc35cc7 commented 4 years ago

Changed keywords from SI2019 to SI2019, sd104

67c74138-1ca1-45a7-bae8-3eb17fc35cc7 commented 4 years ago

comment:11

Resolved the merge conflict

New commits:

cfcd6fe Merge branch 'u/gh-HTalbott/28173_newton_modified' of git://trac.sagemath.org/sage into 28173

fchapoton commented 4 years ago

comment:12

Is the removal of a full method an intended effect ? Just to be sure...

67c74138-1ca1-45a7-bae8-3eb17fc35cc7 commented 4 years ago

comment:13

Yes, the method we removed is part of another ticket and shouldn't have been included here.

fchapoton commented 4 years ago

comment:14

ok then. Next time, think about using "Return" and not "Returns"

vbraun commented 4 years ago

comment:15

Merge conflict

embray commented 4 years ago

comment:16

Ticket retargeted after milestone closed

mkoeppe commented 4 years ago

comment:17

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

tscrim commented 4 years ago

Changed branch from u/atowsley/28173_newton_modified to u/tscrim/is_newton-28173

tscrim commented 4 years ago

Changed reviewer from Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley to Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley, Travis Scrimshaw

tscrim commented 4 years ago

Changed commit from cfcd6fe to 4ecfe28

tscrim commented 4 years ago

comment:18

I made a few small tweaks and changes, but mostly just resolved the merge conflict. Frédéric, can you give a quick check of my changes before setting back to a positive review?

New commits:

`a6d795f`	`Merge branch 'u/atowsley/28173_newton_modified' of git://trac.sagemath.org/sage into u/atowsley/28173_newton_modified`
`4ecfe28`	`Some touchups to the doc and PEP8.`

fchapoton commented 4 years ago

comment:19

Looks good, but there remains one typo :

"if the maps has" should probably be "if the map has"

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

da59247 Fixing typo.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 4ecfe28 to da59247

tscrim commented 4 years ago

comment:21

Thank you. I am interpreting that as a positive review. Please revert if you disagree.

tscrim commented 4 years ago

Changed reviewer from Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley, Travis Scrimshaw to Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley, Travis Scrimshaw, Frédéric Chapoton

vbraun commented 4 years ago

Changed branch from u/tscrim/is_newton-28173 to da59247

slel commented 4 years ago

Changed commit from da59247 to none

sagemath / sage

Implement is_newton for dynamical systems #28173