Opened 3 years ago

Closed 3 years ago

#20780 closed enhancement (fixed)

add level parameter to rational_preimages for projective points

Reported by: bhutz Owned by: bhutz
Priority: minor Milestone: sage-7.3
Component: algebraic geometry Keywords:
Cc: Merged in:
Authors: Ben Hutz Reviewers: Grayson Jorgenson, Rebecca Lauren Miller
Report Upstream: N/A Work issues:
Branch: 6377a27 (Commits) Commit: 6377a27d9be3cf3b46657d294da38f88ee73d65e
Dependencies: Stopgaps:


When working with preimage trees it is usefull to compute more than just the first preimages of a given point. The ticket is to add a parameter to rational_preimages() to specify how far back to find preimages. I.e. find all points Q with f^k(Q) =P for specified k.

Change History (6)

comment:1 Changed 3 years ago by bhutz

  • Authors set to Ben Hutz
  • Branch set to u/bhutz/t/20780
  • Commit set to e7db093e5e6d70000c9806e1555cb09ce579e997
  • Status changed from new to needs_review

New commits:

e7db09320780: add level parameter to rational_preimages

comment:2 Changed 3 years ago by gjorgenson

  • Reviewers set to Grayson Jorgenson

I think this looks good. The code makes sense and appears to work as intended. I just found some really minor things:

  • line 2780 - change TypeError? message to all lowercase
  • Old issues present before this ticket that maybe could be addressed here:
  • line 2680 - `False` to ``False``
  • line 2711 - some spacing in ProjectiveSpace? calls and in lists of defining polynomials

  • line 182 - the sorted() function takes any iterable and returns a list, so it shouldn't be necessary to convert rat_points to a list first

  • line 3920 spacing around '>'
  • lines 3790 - 3793, formatting issues: ``k``th and `k`th don't seem to render well, and perhaps `P` and `Q` should be replaced with ``P``, ``Q``.
  • some ProjectiveSpace? calls in examples of rational_preimages don't have any spacing in the arguments

comment:3 Changed 3 years ago by git

  • Commit changed from e7db093e5e6d70000c9806e1555cb09ce579e997 to 6377a27d9be3cf3b46657d294da38f88ee73d65e

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

6377a2720780: minor fixes

comment:4 Changed 3 years ago by gjorgenson

Okay, this looks fine to me now.

comment:5 Changed 3 years ago by rlmiller

  • Reviewers changed from Grayson Jorgenson to Grayson Jorgenson, Rebecca Lauren Miller
  • Status changed from needs_review to positive_review

Looks good to me.

comment:6 Changed 3 years ago by vbraun

  • Branch changed from u/bhutz/t/20780 to 6377a27d9be3cf3b46657d294da38f88ee73d65e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.