Opened 2 years ago

Closed 20 months ago

# Implement is_newton for dynamical systems

Reported by: Owned by: gh-jl0128 minor sage-9.1 dynamics SI2019, sd104 Anna Chlopecki, Simon Xu, Juliano Levier-Gomes, Grayson Jorgenson Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley, Travis Scrimshaw, Frédéric Chapoton N/A da59247

### Description

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.

### comment:1 Changed 2 years ago by gh-annanc2

• Branch set to u/gh-annanc2/test

### comment:2 Changed 2 years ago by gh-jl0128

• Status changed from new to needs_review

New commits:

 ​408c3ad `28173 : Added is_Newton function.`

### comment:3 Changed 2 years ago by gh-Torrencem

• Reviewers set to Talia Blum, Matt Torrence
• Status changed from needs_review to needs_work
1. The NotImplementedError? ("Degree one Newton maps are trivial.") should not start with a capital letter and shouldn't end with punctuation
1. 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`

1. 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)

1. 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
1. 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?

### comment:4 Changed 2 years ago by gh-ksldr

• Branch changed from u/gh-annanc2/test to u/gh-ksldr/28173_newton
• Commit changed from 408c3add2775e452427c5aff1bf712a1acbc57a0 to c42c414e03c2c597c060c791fa00537004d5b519
• Status changed from needs_work to needs_review

New commits:

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

### comment:5 Changed 2 years ago by gh-HTalbott

• Branch changed from u/gh-ksldr/28173_newton to u/gh-HTalbott/28173_newton

### comment:6 Changed 2 years ago by gh-Torrencem

• Branch changed from u/gh-HTalbott/28173_newton to u/gh-HTalbott/28173_newton_modified
• Commit changed from c42c414e03c2c597c060c791fa00537004d5b519 to 1c552490de91354b39ec9c402d2ac9c3bf73c256

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`

### comment:7 Changed 2 years ago by gh-Torrencem

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

### comment:8 Changed 2 years ago by gh-HTalbott

• Reviewers changed from Talia Blum, Matt Torrence to Talia Blum, Matt Torrence, Henry Talbott

### comment:9 Changed 2 years ago by gjorgenson

This looks good to me, thanks!

### comment:10 Changed 2 years ago by atowsley

• Branch changed from u/gh-HTalbott/28173_newton_modified to u/atowsley/28173_newton_modified

### comment:11 Changed 2 years ago by atowsley

• Commit changed from 1c552490de91354b39ec9c402d2ac9c3bf73c256 to cfcd6fe245e039eec1dc39091210033a3bc5f397
• Reviewers changed from Talia Blum, Matt Torrence, Henry Talbott to Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley
• Status changed from needs_review to positive_review

Resolved the merge conflict

New commits:

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

### comment:12 Changed 2 years ago by chapoton

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

### comment:13 Changed 2 years ago by atowsley

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

### comment:14 Changed 2 years ago by chapoton

• Milestone changed from sage-8.9 to sage-9.0

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

### comment:15 Changed 2 years ago by vbraun

• Status changed from positive_review to needs_work

Merge conflict

### comment:16 Changed 2 years ago by embray

• Milestone changed from sage-9.0 to sage-9.1

Ticket retargeted after milestone closed

### comment:17 Changed 20 months ago by mkoeppe

• Milestone changed from sage-9.1 to sage-9.2

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.

### comment:18 Changed 20 months ago by tscrim

• Branch changed from u/atowsley/28173_newton_modified to u/tscrim/is_newton-28173
• Reviewers changed from Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley to Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley, Travis Scrimshaw
• Status changed from needs_work to needs_review

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.`

### comment:19 Changed 20 months ago by chapoton

Looks good, but there remains one typo :

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

### comment:20 Changed 20 months ago by git

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

 ​da59247 `Fixing typo.`

### comment:21 Changed 20 months ago by tscrim

• Reviewers changed 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
• Status changed from needs_review to positive_review

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

### comment:22 Changed 20 months ago by vbraun

• Branch changed from u/tscrim/is_newton-28173 to da59247229450f9e42a2abd873a73b1244d3b4ed
• Resolution set to fixed
• Status changed from positive_review to closed

### comment:23 Changed 20 months ago by slelievre

• Commit da59247229450f9e42a2abd873a73b1244d3b4ed deleted
• Milestone changed from sage-9.2 to sage-9.1
• Summary changed from Implemented Is_Newton for Dynamical Systems to Implement is_newton for dynamical systems
Note: See TracTickets for help on using tickets.