Opened 3 years ago

Closed 3 years ago

#27779 closed enhancement (fixed)

Py3: Fix dynamics.arithmetic_dynamics for python3

Reported by: vklein Owned by:
Priority: major Milestone: sage-8.8
Component: python3 Keywords:
Cc: bhutz Merged in:
Authors: Vincent Klein Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: e26a443 (Commits, GitHub, GitLab) Commit: e26a443fba787f4a66c0c1b76b84558b61987489
Dependencies: Stopgaps:

Status badges

Description

Fix dynamics.arithmetic_dynamics for python3. One failling doctest remains.

Change History (16)

comment:1 Changed 3 years ago by vklein

  • Branch set to u/vklein/py3__fix_dynamics_arithmetic_dynamics_for_python3

comment:2 Changed 3 years ago by vklein

  • Commit set to ccf846c2be2b5015a2595370475433c4c5aeb1c7
  • Status changed from new to needs_review

New commits:

​ccf846cTrac 27779: Fix arithmetic_dynamics for py3

comment:3 Changed 3 years ago by chapoton

some of the new lines must be tagged # long time too, as they depend on #long results 

+            sage: lst, label = f.automorphism_group(return_functions=True, iso_type=True) # long time
+            sage: sorted(lst, key=str), label

comment:4 Changed 3 years ago by git

  • Commit changed from ccf846c2be2b5015a2595370475433c4c5aeb1c7 to b04931f9f85f85e97701acc3184cc11da459d3d3

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

​b04931fTrac #27779: Add missing `# long time` flag

comment:5 Changed 3 years ago by vklein

Thanks ! Missing tag added.

comment:6 Changed 3 years ago by chapoton

maybe add # abs tol 1e-11 to the last failing doctest ?

comment:7 Changed 3 years ago by vklein

It works. You mean values (x^4 - 2.86348722511320e-12*y^4 : 1.41421356237310*y^4) and (x^4 + 7.74491581978509e-13*y^4 : 1.41421356237310*y^4) are both valid results for this doctest ?

comment:8 Changed 3 years ago by chapoton

I thought so, but maybe this is not so clear. We should cc the authors of that piece of code.

comment:9 Changed 3 years ago by vklein

Git blame says it's from Ben Hutz but he is not listed in sage account list.

comment:10 Changed 3 years ago by git

  • Commit changed from b04931f9f85f85e97701acc3184cc11da459d3d3 to d2ad26030d76568687134cc5536998a28c5a3b4a

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

​d2ad260Trac #27779: Add `# abs tol 1e-11` tag to doctest

comment:11 Changed 3 years ago by chapoton

  • Cc bhutz added

comment:12 Changed 3 years ago by bhutz

Perhaps a cleaner fix here would be to change the doctest to 

g,m = f.reduced_form(smallest_coeffs=False); m

The matrix is it producing for the conjugation is always an integer matrix, but just working over RR to conjugate and then conjugate by the inverse is introducing numerical error, so the resulting function is not an exact match of the original. So, the error that is cropping up in the doctest actually has nothing to do with the functionality of .reduced_form(), rather it is just a consequence of conjugating over a numerical field.

You should probably do the same thing for the doctest immediately following as I would have expected the same kind of issue over CC.

comment:13 Changed 3 years ago by git

  • Commit changed from d2ad26030d76568687134cc5536998a28c5a3b4a to e26a443fba787f4a66c0c1b76b84558b61987489

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

​e26a443Trac #27779: Test the matrix result only for ...

comment:14 Changed 3 years ago by vklein

Ok it's done.
Thank you for your help Ben.

comment:15 Changed 3 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

Ça me parait correct.

comment:16 Changed 3 years ago by vbraun

  • Branch changed from u/vklein/py3__fix_dynamics_arithmetic_dynamics_for_python3 to e26a443fba787f4a66c0c1b76b84558b61987489
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.