add support for dynamical systems over function field in function periodic_points

sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.

https://www.sagemath.org

Other

1.19k stars 412 forks source link

add support for dynamical systems over function field in function periodic_points #31566

Open 2889fc47-50c1-4c1a-bb91-f80beb6916b6 opened 3 years ago

2889fc47-50c1-4c1a-bb91-f80beb6916b6 commented 3 years ago

we modified the if condition so that it doesn't throw an error when working with dynamical systems over function field, the actual code of the function is not changed(except the if condition) because it works on dynamical systems over function field

CC: @bhutz

Component: dynamics

Author: Saher Amasha,Safa Amasha

Branch/Commit: u/gh-Saher-Amasha/add_support_for_dynamical_systems_over_function_field_in_function_periodic_points @ 6fade29

Reviewer: Travis Scrimshaw

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

mkoeppe commented 3 years ago

comment:1

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

2889fc47-50c1-4c1a-bb91-f80beb6916b6 commented 3 years ago

Branch: u/gh-Saher-Amasha/add_support_for_dynamical_systems_over_function_field_in_function_periodic_points

2889fc47-50c1-4c1a-bb91-f80beb6916b6 commented 3 years ago

New commits:

51f9c72 modified the if condition

2889fc47-50c1-4c1a-bb91-f80beb6916b6 commented 3 years ago

Commit: 51f9c72

tscrim commented 3 years ago

comment:4

Why are you checking the base ring and not the ring R?

tscrim commented 3 years ago

Reviewer: Travis Scrimshaw

bhutz commented 3 years ago

comment:5

Yes, you should be checking R to save an extra call of .base-ring()

You also need to add at least one example.

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

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

6fade29 added 1 example,saved an extra call of .base-ring()

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

Changed commit from 51f9c72 to 6fade29

mkoeppe commented 2 years ago

comment:7

Setting a new milestone for this ticket based on a cursory review.

EnderWannabe commented 2 years ago

comment:8

Merge conflict on the latest release.

Also, the example should include a t, so that the map is properly defined over the function field.

The latest release also throws an unnecessary ValueError when minimal=False over function fields. This can be fixed in this ticket or a separate ticket.