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: |
Description (last modified by )
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)
Change History (43)
comment:1 Changed 5 years ago by
- Branch set to u/rlmiller/stoll
comment:2 Changed 5 years ago by
- Commit set to 35cf9f14523e2cdf082b1ebca5da9e23ef33fbd7
- Status changed from new to needs_review
comment:3 Changed 5 years ago by
- Commit changed from 35cf9f14523e2cdf082b1ebca5da9e23ef33fbd7 to c8fb08609a46ce288aa08ebce731827b86198ccb
Branch pushed to git repo; I updated commit sha1. New commits:
c8fb086 | 23693 updated points() to work over CC in affine_homset.py
|
comment:4 Changed 5 years ago by
- Priority changed from major to minor
comment:5 Changed 5 years ago by
- 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
- 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
- Branch changed from u/rlmiller/stoll to u/raghukul01/stoll
Rebase from current master
comment:8 Changed 4 years ago by
- Commit changed from c8fb08609a46ce288aa08ebce731827b86198ccb to 1c5dba997e51df604c97a1e6e2b6f02600d3fb2d
Branch pushed to git repo; I updated commit sha1. New commits:
1c5dba9 | Added Support for CDF
|
comment:9 Changed 4 years ago by
- Keywords gsoc2018 added
comment:10 Changed 4 years ago by
- Commit changed from 1c5dba997e51df604c97a1e6e2b6f02600d3fb2d to fa9418f725a92a26aa0940726020718e5981767e
comment:11 Changed 4 years ago by
- Commit changed from fa9418f725a92a26aa0940726020718e5981767e to e4f49f538dbf55f36c3f2cd1cca427cd0586f3b3
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e4f49f5 | 23627 Added support to handle duplicates
|
comment:12 follow-up: ↓ 13 Changed 4 years ago by
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
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)):
comment:14 Changed 4 years ago by
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
- Commit changed from e4f49f538dbf55f36c3f2cd1cca427cd0586f3b3 to dc753b831f0135052e09e105f7484bd54a63e74c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
dc753b8 | 23627 Added support to handle duplicates
|
comment:16 Changed 4 years ago by
- Branch changed from u/raghukul01/stoll to u/bhutz/stoll
comment:17 Changed 4 years ago by
- 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:
3a02dd0 | 23627: add numerical point support
|
comment:18 Changed 4 years ago by
- Cc Ben Hutz removed
- Milestone changed from sage-8.1 to sage-8.3
comment:19 Changed 4 years ago by
- Reviewers changed from Ben Hutz to Ben Hutz, Raghukul Raman
comment:20 Changed 4 years ago by
- Branch changed from u/bhutz/stoll to u/raghukul01/stoll
comment:21 Changed 4 years ago by
- 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:
dbb9326 | Reviewier's patch #23627
|
comment:22 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:23 Changed 4 years ago by
- 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:
79b5e0e | Merge branch 'u/raghukul01/stoll' of git://trac.sagemath.org/sage into 23627
|
7c09986 | 23627: changes from review
|
comment:24 Changed 4 years ago by
- Status changed from needs_review to positive_review
Those 2 examples were so silly of me :P
comment:25 Changed 4 years ago by
- 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
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
- Commit changed from 7c099867bb615147a2b4bef077b2d51705c42a48 to 537ecbd222bd1900b0d55170e61a09cc98c60a9e
Branch pushed to git repo; I updated commit sha1. New commits:
537ecbd | 23627: fix doctest
|
comment:27 Changed 4 years ago by
- 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
- Status changed from needs_review to positive_review
Passed on my system as well.
comment:30 Changed 4 years ago by
- Commit changed from 537ecbd222bd1900b0d55170e61a09cc98c60a9e to d076ca9a084e35a0fd35911c9a4a3440acb25a1a
Branch pushed to git repo; I updated commit sha1. New commits:
d076ca9 | Merge branch 8.3.beta6 into 23627
|
comment:31 Changed 4 years ago by
- Status changed from needs_work to needs_review
merge conflict resolved
comment:32 Changed 4 years ago by
- 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
comment:33 Changed 4 years ago by
- 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
- Commit changed from d076ca9a084e35a0fd35911c9a4a3440acb25a1a to 84b290a7e285a09c9bd1b6701add2aa16851d2f5
Branch pushed to git repo; I updated commit sha1. New commits:
84b290a | 23627: fix doctests for 32bit
|
comment:35 Changed 4 years ago by
- Status changed from needs_work to needs_review
corrected a minor whitespace typo as well.
comment:36 Changed 4 years ago by
How can I run doctest for a 32-bit machine? Mine is 64.
comment:38 Changed 4 years ago by
- 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
- Commit changed from 84b290a7e285a09c9bd1b6701add2aa16851d2f5 to c6f8d4f64a80a5e17bc880b03587ee05ae421e45
comment:40 Changed 4 years ago by
- Status changed from needs_work to needs_review
merged in and updated doctests for latest beta
comment:41 Changed 4 years ago by
- Status changed from needs_review to positive_review
comment:42 Changed 4 years ago by
- Branch changed from u/bhutz/23627_numerical_points to c6f8d4f64a80a5e17bc880b03587ee05ae421e45
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
23627 update points() to work over CC