Opened 5 years ago

Closed 4 years ago

#23627 closed enhancement (fixed)

Update points() in projective_homset.py and affine_homset.py to work over CC and CDF

Reported by: rlmiller Owned by:
Priority: minor Milestone: sage-8.3
Component: algebraic geometry Keywords: gsoc2018
Cc: Paul, Fili Merged in:
Authors: Rebecca Lauren Miller, Raghukul Raman, Ben Hutz Reviewers: Ben Hutz, Raghukul Raman
Report Upstream: N/A Work issues:
Branch: c6f8d4f (Commits, GitHub, GitLab) Commit: c6f8d4f64a80a5e17bc880b03587ee05ae421e45
Dependencies: Stopgaps:

Status badges

Description (last modified by rlmiller)

Updated numerical point finding in the projective_homset.py and affine homset.py files to work over the complex numbers and the complex double field.

Attachments (1)

output.txt (615.7 KB) - added by raghukul01 4 years ago.
I guess most of the doc test are failing due to following line in sage/coding/source-coding/hamming_code.py PFn = [list(p) for p in X.point_set(F).points(F)] (line no 147)

Download all attachments as: .zip

Change History (43)

comment:1 Changed 5 years ago by rlmiller

  • Branch set to u/rlmiller/stoll

comment:2 Changed 5 years ago by rlmiller

  • Commit set to 35cf9f14523e2cdf082b1ebca5da9e23ef33fbd7
  • Status changed from new to needs_review

New commits:

35cf9f123627 update points() to work over CC

comment:3 Changed 5 years ago by git

  • Commit changed from 35cf9f14523e2cdf082b1ebca5da9e23ef33fbd7 to c8fb08609a46ce288aa08ebce731827b86198ccb

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

c8fb08623693 updated points() to work over CC in affine_homset.py

comment:4 Changed 5 years ago by rlmiller

  • Priority changed from major to minor

comment:5 Changed 5 years ago by bhutz

  • Component changed from dynamics to algebraic geometry
  • Reviewers set to Ben Hutz
  • Status changed from needs_review to needs_work
  • Type changed from PLEASE CHANGE to enhancement

This ticket and #23693 have the same branch and contains both the changes for affine_homset and projective_homset. What did you mean to do with these two tickets?

Otherwise, just a few minor things:

  • CDF should work in addition to CC
  • a couple of the comments in the affine_homset reference projective. Fix those.
  • the sorting line is commented out in affine_homset and should be restored
  • You need to update the docmentation of .rational_points() to describe the changes and add an example there.

comment:6 Changed 5 years ago by rlmiller

  • Description modified (diff)
  • Summary changed from Update points() in projective_homset.py to work over CC to Update points() in projective_homset.py and affine_homset.py to work over CC and CDF

comment:7 Changed 4 years ago by raghukul01

  • Branch changed from u/rlmiller/stoll to u/raghukul01/stoll

Rebase from current master

Last edited 4 years ago by raghukul01 (previous) (diff)

comment:8 Changed 4 years ago by git

  • Commit changed from c8fb08609a46ce288aa08ebce731827b86198ccb to 1c5dba997e51df604c97a1e6e2b6f02600d3fb2d

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

1c5dba9Added Support for CDF

comment:9 Changed 4 years ago by raghukul01

  • Authors changed from Rebecca Lauren Miller to Rebecca Lauren Miller, Raghukul Raman
  • Keywords gsoc2018 added

comment:10 Changed 4 years ago by git

  • Commit changed from 1c5dba997e51df604c97a1e6e2b6f02600d3fb2d to fa9418f725a92a26aa0940726020718e5981767e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e5e83edAdded Support for CDF
fa9418f#23627 Added support to handle duplicates

comment:11 Changed 4 years ago by git

  • Commit changed from fa9418f725a92a26aa0940726020718e5981767e to e4f49f538dbf55f36c3f2cd1cca427cd0586f3b3

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e4f49f523627 Added support to handle duplicates

comment:12 follow-up: Changed 4 years ago by bhutz

Just a couple simple comments before I test functionality.

Instead of importing, its better to use members function whenever possible (i.e., .floor())

The two for loops

for i in range(len(dupl_points)):
    for j in range(len(dupl_points)):
        if j > i:
            u = dupl_points[i]

can be simplified to

for i in range(len(dupl_points)):
    u = dupl_points[i]
    for j in range(i+1, len(dupl_points)):

comment:13 in reply to: ↑ 12 Changed 4 years ago by raghukul01

Actually after product with tolerance, the result becomes float, and hence floor(), member function doesn't exist. So there are 2 options

1) Convert tolerance to RealNumber? and then use .floor()

2) use floor from math

currently I implemented using 2), should I change to 1)? or do you have any other idea?

Thanks

Replying to bhutz:

Just a couple simple comments before I test functionality.

Instead of importing, its better to use members function whenever possible (i.e., .floor())

The two for loops

for i in range(len(dupl_points)):
    for j in range(len(dupl_points)):
        if j > i:
            u = dupl_points[i]

can be simplified to

for i in range(len(dupl_points)):
    u = dupl_points[i]
    for j in range(i+1, len(dupl_points)):
Last edited 4 years ago by raghukul01 (previous) (diff)

comment:14 Changed 4 years ago by bhutz

I would do 1) since you should do basic input checking on tolerance anyway, so you can convert to RealField? and check that it's positive.

comment:15 Changed 4 years ago by git

  • Commit changed from e4f49f538dbf55f36c3f2cd1cca427cd0586f3b3 to dc753b831f0135052e09e105f7484bd54a63e74c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

dc753b823627 Added support to handle duplicates

comment:16 Changed 4 years ago by bhutz

  • Branch changed from u/raghukul01/stoll to u/bhutz/stoll

comment:17 Changed 4 years ago by bhutz

  • Authors changed from Rebecca Lauren Miller, Raghukul Raman to Rebecca Lauren Miller, Raghukul Raman, Ben Hutz
  • Commit changed from dc753b831f0135052e09e105f7484bd54a63e74c to 3a02dd05fa3457ff1c3a713e28adc5cd9e000236
  • Status changed from needs_work to needs_review

in testing, I found that the method does very poorly when the base ring is numerical such as CC or RR. For simple subschemes, it was ok, but for even mildly complicated schemes (such as projective closures of intersections of affine curves) you could easily get wildly wrong answers. So, I think the verbose warning is warranted in this case.

This also led me to separate out the case where the subscheme is defined over a number field and the user wants points numerically approximated which is much faster. This seems to work well in my testing (up to precision/tolerance).

Additionally, the documenation was significantly updated as well as normalizing all the inputs for the various points/rational_points methods.

So I'm marking this as needs-review, but it should undergo fairly extensive testing as I'm sure there are cases I missed.


New commits:

3a02dd023627: add numerical point support

comment:18 Changed 4 years ago by bhutz

  • Cc Ben Hutz removed
  • Milestone changed from sage-8.1 to sage-8.3

comment:19 Changed 4 years ago by raghukul01

  • Reviewers changed from Ben Hutz to Ben Hutz, Raghukul Raman

comment:20 Changed 4 years ago by raghukul01

  • Branch changed from u/bhutz/stoll to u/raghukul01/stoll

comment:21 Changed 4 years ago by raghukul01

  • Commit changed from 3a02dd05fa3457ff1c3a713e28adc5cd9e000236 to dbb93269762b229edf6c4b5d19b9b074ba3f40a4

There are couple of documentation updated

  • Value of dimension need to be corrected (line no := for projective_homset 74, 188, 383, 453; for affine_homset 129, 138).
  • Calculation of dimension in projective homset is wrong (needs to be one less than I.dimension(), line no :- 185, 380).
  • Subscheme should be replaced by scheme (line no: for projective homset 75, 293; for affine homset 129, 317).
  • Projective word should be replaced by affine in affine homset (line no := 283, 436).
  • Example in docstring of projective_homset for CC, and CDF are similar. different example need to be added.
  • For NumberFields embeddings should be specified during formation, this needs to be added in documentation.
  • In affine homset example below is better to illustrate working in CDF, compared to current example
    sage: A.<x1, x2> = AffineSpace(CDF, 2)
    sage: E = A.subscheme([x1^2 + x2^2 + x1*x2 + 1, x1 + x2])
    sage: E(A.base_ring()).points() 
    verbose 0 (124: affine_homset.py, points) Warning: computations in the
    numerical fields are inexact;points may be computed partially or
    incorrectly.
    [(-2.77555756156e-17 + 1.0*I, 2.77555756156e-17 - 1.0*I), (-1.0*I,
    1.0*I)]
    
  • There should be an example in projective and affine homset to illustrate use of tolerance, some thing like this
    sage: A.<x1, x2> = AffineSpace(QQ, 2)
    sage: E = A.subscheme([x1^1000 + x2^2 + x1*x2 + 1, x1 + x2])
    sage: len(E(A.base_ring()).numerical_points(F=CDF, zero_tolerance =1e-11))
    987
    sage: len(E(A.base_ring()).numerical_points(F=CDF, zero_tolerance =1e-10))
    1000
    

Following gives wrong result

sage: P.<x1, x2> = ProjectiveSpace(QQ, 1)
sage: E = A.subscheme([x1^2 -3*x1 + 2, x2])
sage: E(A.base_ring()).numerical_points(F=CDF, zero_tolerance =1e-10)
[(1.0, 0.0), (2.0, 0.0)]
sage: E(A.base_ring()).points()
[(1, 0), (2, 0)]

They are same projective point, and hence only 1 should be returned.

This also

sage: P.<x1> = ProjectiveSpace(QQ, 0)
sage: E = A.subscheme([x1^2 -3*x1 + 2])
sage: E(A.base_ring()).points(bound=5)
[(1, -5), (1, -4), (1, -3), (1, -2), (1, -1), (1, 0), (1, 1), (1, 2),
(1, 3), (1, 4), (1, 5), (2, -5), (2, -4), (2, -3), (2, -2), (2, -1), (2,
0), (2, 1), (2, 2), (2, 3), (2, 4), (2, 5)]

New commits:

dbb9326Reviewier's patch #23627
Last edited 4 years ago by raghukul01 (previous) (diff)

comment:22 Changed 4 years ago by raghukul01

  • Status changed from needs_review to needs_work

comment:23 Changed 4 years ago by bhutz

  • Branch changed from u/raghukul01/stoll to u/bhutz/23627_numerical_points
  • Commit changed from dbb93269762b229edf6c4b5d19b9b074ba3f40a4 to 7c099867bb615147a2b4bef077b2d51705c42a48
  • Status changed from needs_work to needs_review

Changes made. A few comment below

  • The dimension checks are actually correct, that is why the comments are there
  • made doc changes where appropriate (can't really have a 0 dim scheme in sage)
  • I modified the examples as specified
  • However, the two examples above that are 'failing' are actually problems with the examples. Notice that you are defining a project space P and using subschemes of A.

New commits:

79b5e0eMerge branch 'u/raghukul01/stoll' of git://trac.sagemath.org/sage into 23627
7c0998623627: changes from review

comment:24 Changed 4 years ago by raghukul01

  • Status changed from needs_review to positive_review

Those 2 examples were so silly of me :P

comment:25 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Test failures... Always run 'make ptestlong' before setting stuff to positive review.

Changed 4 years ago by raghukul01

I guess most of the doc test are failing due to following line in sage/coding/source-coding/hamming_code.py PFn = [list(p) for p in X.point_set(F).points(F)] (line no 147)

comment:26 Changed 4 years ago by git

  • Commit changed from 7c099867bb615147a2b4bef077b2d51705c42a48 to 537ecbd222bd1900b0d55170e61a09cc98c60a9e

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

537ecbd23627: fix doctest

comment:27 Changed 4 years ago by bhutz

  • Status changed from needs_work to needs_review

looks like hamming_codes was calling .points() and trying to pass in the field. This was never possible in the first place (only could pass in bound or precision) it was only coming up since the keyword parameters were moved to *kwds and not listed explicitly.

This one change fixed all doctests for me.

comment:28 Changed 4 years ago by raghukul01

  • Status changed from needs_review to positive_review

Passed on my system as well.

comment:29 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

merge conflict

comment:30 Changed 4 years ago by git

  • Commit changed from 537ecbd222bd1900b0d55170e61a09cc98c60a9e to d076ca9a084e35a0fd35911c9a4a3440acb25a1a

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

d076ca9Merge branch 8.3.beta6 into 23627

comment:31 Changed 4 years ago by bhutz

  • Status changed from needs_work to needs_review

merge conflict resolved

comment:32 Changed 4 years ago by raghukul01

  • Status changed from needs_review to positive_review

Following doctest are failing however they are not due to this ticket, they are failing on the develop(8.3.beta7) branch as well.

sage -t --long --warn-long 91.1 src/sage/tests/cmdline.py  # 1 doctest failed
sage -t --long --warn-long 91.1 src/sage/functions/other.py  # 1 doctest failed
sage -t --long --warn-long 91.1 src/sage/rings/complex_arb.pyx  # 24 doctests failed
sage -t --long --warn-long 91.1 src/sage/misc/sphinxify.py  # 1 doctest failed
sage -t --long --warn-long 91.1 src/sage/rings/real_arb.pyx  # 7 doctests failed
sage -t --long --warn-long 91.1 src/sage/matrix/matrix_complex_ball_dense.pyx  # 1 doctest failed
sage -t --long --warn-long 91.1 src/sage/rings/polynomial/polynomial_complex_arb.pyx  # 2 doctests failed
Last edited 4 years ago by raghukul01 (previous) (diff)

comment:33 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

On 32-bit

**********************************************************************
File "src/sage/schemes/projective/projective_homset.py", line 160, in sage.schemes.projective.projective_homset.SchemeHomset_points_projective_field.points
Failed example:
    E(P.base_ring()).points()
Expected:
    verbose 0 (70: projective_homset.py, points) Warning: computations in the numerical fields are inexact;points may be computed partially or incorrectly.
    [(-1.0000000000000004*I : 1.0 : 0.0),
     (0.0 : 0.9999999999999997*I : 1.0),
     (0.0 : 2.7755575615628914e-17 - 1.0*I : 1.0),
     (0.9999999999999997*I : 1.0 : 0.0),
     (2.7755575615628914e-17 - 1.0*I : 0.0 : 1.0),
     (2.7755575615628914e-17 + 1.0*I : 0.0 : 1.0)]
Got:
    verbose 0 (70: projective_homset.py, points) Warning: computations in the numerical fields are inexact;points may be computed partially or incorrectly.
    [(-1.3091741232762466e-17 - 1.0*I : 0.0 : 1.0),
     (-1.3091741232762466e-17 + 1.0*I : 0.0 : 1.0),
     (-1.0000000000000002*I : 1.0 : 0.0),
     (0.0 : -1.0000000000000002*I : 1.0),
     (0.0 : 0.9999999999999998*I : 1.0),
     (0.9999999999999998*I : 1.0 : 0.0)]
**********************************************************************
File "src/sage/schemes/projective/projective_homset.py", line 335, in sage.schemes.projective.projective_homset.SchemeHomset_points_projective_field.numerical_points
Failed example:
    X(K).numerical_points(F=CDF)
Expected:
    [(-1.475773161594552 : 1.475773161594552 : 1.0),
     (1.475773161594551 : 1.4757731615945517 : 1.0)]
Got:
    [(-1.475773161594552 : 1.475773161594552 : 1.0),
     (1.4757731615945517 : 1.4757731615945517 : 1.0)]
**********************************************************************
2 items had failures:
   1 of  22 in sage.schemes.projective.projective_homset.SchemeHomset_points_projective_field.numerical_points
   1 of  20 in sage.schemes.projective.projective_homset.SchemeHomset_points_projective_field.points
    [75 tests, 2 failures, 1.65 s]
----------------------------------------------------------------------
sage -t --long src/sage/schemes/projective/projective_homset.py  # 2 doctests failed
----------------------------------------------------------------------

comment:34 Changed 4 years ago by git

  • Commit changed from d076ca9a084e35a0fd35911c9a4a3440acb25a1a to 84b290a7e285a09c9bd1b6701add2aa16851d2f5

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

84b290a23627: fix doctests for 32bit

comment:35 Changed 4 years ago by bhutz

  • Status changed from needs_work to needs_review

corrected a minor whitespace typo as well.

comment:36 Changed 4 years ago by raghukul01

How can I run doctest for a 32-bit machine? Mine is 64.

comment:37 Changed 4 years ago by raghukul01

  • Status changed from needs_review to positive_review

tested, LGTM

comment:38 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work
sage -t --long src/sage/schemes/projective/projective_homset.py
**********************************************************************
File "src/sage/schemes/projective/projective_homset.py", line 146, in sage.schemes.projective.projective_homset.SchemeHomset_points_projective_field.points
Failed example:
    L=E(P.base_ring()).points();L
Expected:
    verbose 0 (70: projective_homset.py, points) Warning: computations in the numerical fields are inexact;points may be computed partially or incorrectly.
    [(-0.500000000000000 + 0.866025403784439*I : 1.00000000000000 : 0.000000000000000),
    (-0.500000000000000 - 0.866025403784439*I : 1.00000000000000 : 0.000000000000000),
    (-1.00000000000000*I : 0.000000000000000 : 1.00000000000000),
    (0.000000000000000 : 0.000000000000000 : 1.00000000000000),
    (1.00000000000000*I : 0.000000000000000 : 1.00000000000000),
    (1.00000000000000 : 1.00000000000000 : 0.000000000000000)]
Got:
    verbose 0 (71: projective_homset.py, points) Warning: computations in the numerical fields are inexact;points may be computed partially or incorrectly.
    [(-0.500000000000000 + 0.866025403784439*I : 1.00000000000000 : 0.000000000000000),
     (-0.500000000000000 - 0.866025403784439*I : 1.00000000000000 : 0.000000000000000),
     (-1.00000000000000*I : 0.000000000000000 : 1.00000000000000),
     (0.000000000000000 : 0.000000000000000 : 1.00000000000000),
     (1.00000000000000*I : 0.000000000000000 : 1.00000000000000),
     (1.00000000000000 : 1.00000000000000 : 0.000000000000000)]
**********************************************************************
File "src/sage/schemes/projective/projective_homset.py", line 161, in sage.schemes.projective.projective_homset.SchemeHomset_points_projective_field.points

comment:39 Changed 4 years ago by git

  • Commit changed from 84b290a7e285a09c9bd1b6701add2aa16851d2f5 to c6f8d4f64a80a5e17bc880b03587ee05ae421e45

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

f4c1affMerge branch 8.3.rc1 into t/23627/23627_numerical_points
c6f8d4f23627: update doctest

comment:40 Changed 4 years ago by bhutz

  • Status changed from needs_work to needs_review

merged in and updated doctests for latest beta

comment:41 Changed 4 years ago by raghukul01

  • Status changed from needs_review to positive_review

comment:42 Changed 4 years ago by vbraun

  • Branch changed from u/bhutz/23627_numerical_points to c6f8d4f64a80a5e17bc880b03587ee05ae421e45
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.