Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#28173 closed enhancement (fixed)

Implement is_newton for dynamical systems

Reported by: gh-jl0128 Owned by:
Priority: minor Milestone: sage-9.1
Component: dynamics Keywords: SI2019, sd104
Cc: Merged in:
Authors: Anna Chlopecki, Simon Xu, Juliano Levier-Gomes, Grayson Jorgenson Reviewers: Talia Blum, Matt Torrence, Henry Talbott, Adam Towsley, Travis Scrimshaw, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: da59247 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

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.

Change History (23)

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

  • Commit set to 408c3add2775e452427c5aff1bf712a1acbc57a0
  • Status changed from new to needs_review

New commits:

408c3ad28173 : 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:

ed362ffMerge branch 'u/gh-annanc2/test' of git://trac.sagemath.org/sage into newton_change
c42c41428173: 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:

7d8e7caMerge branch 'u/gh-ksldr/28173_newton' of git://trac.sagemath.org/sage into test
1c5524928173: 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
  • Keywords sd104 added
  • 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:

cfcd6feMerge 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
  • Commit changed from cfcd6fe245e039eec1dc39091210033a3bc5f397 to 4ecfe287678f8742a3eaacad0add3f4fbf452229
  • 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:

a6d795fMerge branch 'u/atowsley/28173_newton_modified' of git://trac.sagemath.org/sage into u/atowsley/28173_newton_modified
4ecfe28Some 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

  • Commit changed from 4ecfe287678f8742a3eaacad0add3f4fbf452229 to da59247229450f9e42a2abd873a73b1244d3b4ed

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

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