Opened 22 months ago

Last modified 4 months ago

#31549 needs_work enhancement

add support for dynamical systems over function field in function all_rational_preimages

Reported by: Saher Amasha Owned by:
Priority: minor Milestone: sage-9.8
Component: dynamics Keywords:
Cc: Ben Hutz Merged in:
Authors: Saher Amasha,Safa Amasha Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: u/gh-Saher-Amasha/add_support_for_dynamical_systems_over_function_field_in_function__all_rational_preimages (Commits, GitHub, GitLab) Commit: 46fc21e63abd13164dbbe4b80f6e53c5516ee1aa
Dependencies: Stopgaps:

Status badges

Description (last modified by Saher Amasha)

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 because it works on dynamical systems over function field

Change History (16)

comment:1 Changed 22 months ago by Saher Amasha

Description: modified (diff)

comment:2 Changed 22 months ago by Saher Amasha

Description: modified (diff)

comment:3 Changed 22 months ago by Saher Amasha

Branch: u/gh-Saher-Amasha/add_support_for_dynamical_systems_over_function_field_in_function__all_rational_preimages

comment:4 Changed 22 months ago by Matthias Köppe

Milestone: sage-9.3sage-9.4

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.

comment:5 Changed 22 months ago by Saher Amasha

Commit: 330a6d0f2daaa5493fbc79522d9f07a8dc9c9c86
Status: newneeds_review

New commits:

330a6d0we 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 because it works on dynamical systems over function field

comment:6 Changed 22 months ago by Travis Scrimshaw

Cc: Ben Hutz added

comment:7 Changed 22 months ago by Ben Hutz

I can take a look at this change, but it probably won't be until next week.

just a quick comment. The commit message should be short (one line) with the more detailed description in a following paragraph.

comment:8 Changed 22 months ago by Ben Hutz

Reviewers: Ben Hutz
Status: needs_reviewneeds_work

On the surface this functionality works, but the details need a little work.

In the code itself there are a number of white space issues. For example

[(1/t)*x^2 + y^2  ,y^2])

should be

[(1/t)*x^2 + y^2, y^2])

In the new if line as well, you're missing a space after the comma and have an extra space before the closing ).

The example code should be improved in a number of ways:

  • I'd suggest saving a line and removing K and just defining the function field over the finite field directly.
  • The variables base and CF are not used so should not be in the example.
  • D is not needed: P is already the domain of DS.
  • Instead of calling normalize, just define the dynamical system as you want it to be defined as.

While adding support for function fields, seems like you should also allow finite fields, since that also requires no other changes.

comment:9 Changed 20 months ago by git

Commit: 330a6d0f2daaa5493fbc79522d9f07a8dc9c9c8646fc21e63abd13164dbbe4b80f6e53c5516ee1aa

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

46fc21efixed test

comment:10 Changed 20 months ago by Saher Amasha

Status: needs_workneeds_review

comment:11 Changed 20 months ago by Ben Hutz

Status: needs_reviewneeds_work

There is a doctest failure. Looks like you might need to use "sorted" for that doctest.

Also, why did you not also allow finite fields as requested?

The whitespace issues were fixed in the example, but the example no longer is a true function field example as the function and all the points are over the base field. The example should use the function field variable 't'.

comment:12 Changed 19 months ago by Matthias Köppe

Milestone: sage-9.4sage-9.5

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

comment:13 Changed 18 months ago by Alexander Galarraga

Component: algebradynamics

comment:14 Changed 14 months ago by Matthias Köppe

Milestone: sage-9.5sage-9.6

comment:15 Changed 10 months ago by Matthias Köppe

Milestone: sage-9.6sage-9.7

comment:16 Changed 4 months ago by Matthias Köppe

Milestone: sage-9.7sage-9.8
Note: See TracTickets for help on using tickets.