Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#16986 closed enhancement (fixed)

Enhance rational_preimages and all_rational_preimages to work over Number Fields

Reported by: jdefaria Owned by:
Priority: major Milestone: sage-6.4
Component: algebraic geometry Keywords:
Cc: bhutz Merged in:
Authors: Joao Alberto de Faria Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: 48eebc0 (Commits) Commit:
Dependencies: Stopgaps:

Description

Fixing the checks at the start of the code to allow both functions to work over Number Fields

Change History (22)

comment:1 Changed 4 years ago by jdefaria

  • Branch set to u/jdefaria/ticket/16986
  • Created changed from 09/15/14 15:11:22 to 09/15/14 15:11:22
  • Modified changed from 09/15/14 15:11:22 to 09/15/14 15:11:22

comment:2 Changed 4 years ago by jdefaria

  • Commit set to df867c1288da3a89d8a1df1d9cceb6f5dd5bacb9
  • Status changed from new to needs_review
  • Summary changed from Fix rational_preimages and all_rational_preimages to work over Number Fields to Enhance rational_preimages and all_rational_preimages to work over Number Fields

New commits:

df867c1Added functionality for Number Fields

comment:3 Changed 4 years ago by jdefaria

  • Authors changed from jdefaria to Joao de Faria

comment:4 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

The cases of number fields and finite fields work fine, but many other fields fail. Also there are some issues in the documentation:

  • The descriptions of both functions reference QQ only
  • The description of all_rational_preimages reference parallel (which it is not)
  • The text is after the :: in examples so is not formatted correctly
  • number field is 2 words and not capitalized

For functionality: here are some examples which fail. They fail in at least 3 distinct ways.

R.<t>=FunctionField(QQ)
P.<x,y>=ProjectiveSpace(R,1)
H=End(P)
f=H([x^2-y^2,y^2])
f.rational_preimages(P([0,1]))
R.<t>=PolynomialRing(QQ)
P.<x,y>=ProjectiveSpace(FractionField(R),1)
H=End(P)
f=H([x^2-y^2,y^2])
f.rational_preimages(P([0,1]))
R.<t>=PolynomialRing(QuadraticField(2))
P.<x,y>=ProjectiveSpace(FractionField(R),1)
H=End(P)
f=H([x^2-y^2,y^2])
f.rational_preimages(P([0,1]))
P.<x,y>=ProjectiveSpace(Qp(3),1)
H=End(P)
f=H([x^2-y^2,y^2])
f.rational_preimages(P([0,1]))
P.<x,y>=ProjectiveSpace(CC,1)
H=End(P)
f=H([x^2-y^2,y^2])
f.rational_preimages(P([0,1]))

comment:5 Changed 4 years ago by bhutz

  • Reviewers set to Ben Hutz

comment:6 Changed 4 years ago by git

  • Commit changed from df867c1288da3a89d8a1df1d9cceb6f5dd5bacb9 to 684b502617d603bfdc5e3416aaeaed05499f6f3f

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

18954a9Fixed bug with wrong index being called
684b502Added logic to filter out indeterminate points and subschemes as preimages

comment:7 Changed 4 years ago by jdefaria

  • Status changed from needs_work to needs_review

comment:8 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

Doc test failures:

sage -t projective_morphism.py  # 6 doctests failed

comment:9 Changed 4 years ago by git

  • Commit changed from 684b502617d603bfdc5e3416aaeaed05499f6f3f to 07592c745b9d0c24fd137d95d949ba8130d0a0f0

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

07592c7Fixed a logic error

comment:10 Changed 4 years ago by jdefaria

  • Status changed from needs_work to needs_review

comment:11 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

The code snippents above are resolved now, but here are a couple more issues:

  • the docs are still wrong see comment 4 above
  • for subschemes the points being returned are in the ambient space and not the subscheme
  • the following two examples fail
P.<x,y>=ProjectiveSpace(CC,1)
H=End(P)
f=H([x^2-2*y^2,y^2])
f.rational_preimages(P([0,1]))

The previous example also fails for RR

z = var('z')
K.<w>=NumberField(z^3+2)
R.<z>=PolynomialRing(K)
L.<v>=K.extension(z^2+z+1)
P.<x,y>=ProjectiveSpace(L,1)
H=End(P)
f=H([x^3-2*y^3,v*y^3])
f.rational_preimages(P([0,1]))

comment:12 Changed 4 years ago by git

  • Commit changed from 07592c745b9d0c24fd137d95d949ba8130d0a0f0 to 4c94954f4a055c3a88cb22fe90cb9f05ecc36c33

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

4c94954Fixed documentation, added logic for precision fields

comment:13 Changed 4 years ago by git

  • Commit changed from 4c94954f4a055c3a88cb22fe90cb9f05ecc36c33 to c6bb45162881ef1c2c8472d00e7b0a15e76a5b1d

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

b4e09e9Fixed code to coerce linear factors into proper base rings
c6bb451Added proper imports

comment:14 Changed 4 years ago by git

  • Commit changed from c6bb45162881ef1c2c8472d00e7b0a15e76a5b1d to 928941e6a3df966c89ae97c7bf0250bd049e264c

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

928941eCoerced points into proper domain

comment:15 Changed 4 years ago by jdefaria

  • Status changed from needs_work to needs_review

Fixed all of the problems found above, ran doc test, all docs passed on my end, setting to review

comment:16 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

Actually, this example is not fixed

P.<x,y>=ProjectiveSpace(CC,1)
H=End(P)
f=H([x^2-2*y^2,y^2])
f.rational_preimages(P([0,1]))

but the other one is.

Also some minor things:

  • Both docstrings still says requires QQ and endomorphism of projective space. Neither are still true.
  • There are two lines with terminating white space 2519, 2550.
  • Coordinates is spelled wrong on 2604.

comment:17 Changed 4 years ago by bhutz

opps, my mistake, that example is fixed I just didn't get the latest commits on my last pull. There are still three minor issues:

  • line 2552 trailing white space
  • doc string references endomorphism of projective space, but since you're allowing subschemes, you just need 'an endomorphism'.
  • coordinates is spelled wrong line 2611

comment:18 Changed 4 years ago by git

  • Commit changed from 928941e6a3df966c89ae97c7bf0250bd049e264c to 48eebc0292ffbb4cb4c36f2d9fb159d3bc41f653

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

48eebc0Fixed minor typos

comment:19 Changed 4 years ago by jdefaria

  • Status changed from needs_work to needs_review

comment:20 Changed 4 years ago by bhutz

  • Status changed from needs_review to positive_review

this looks good now.

comment:21 Changed 4 years ago by vbraun

  • Branch changed from u/jdefaria/ticket/16986 to 48eebc0292ffbb4cb4c36f2d9fb159d3bc41f653
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 Changed 4 years ago by jdemeyer

  • Authors changed from Joao de Faria to Joao Alberto de Faria
  • Commit 48eebc0292ffbb4cb4c36f2d9fb159d3bc41f653 deleted
Note: See TracTickets for help on using tickets.