Opened 3 years ago

Closed 3 years ago

#29994 closed enhancement (fixed)

Add integral curves over QQ

Reported by: Kwankyu Lee Owned by:
Priority: minor Milestone: sage-9.2
Component: algebraic geometry Keywords:
Cc: Merged in:
Authors: Kwankyu Lee Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: d9a05b7 (Commits, GitHub, GitLab) Commit: d9a05b78dd6419a4d57daabcc4ee4670edb7bd7f
Dependencies: Stopgaps:

Status badges

Description (last modified by Kwankyu Lee)

We refactor integral curves (over finite fields) code to allow integral curves over QQ.

Change History (9)

comment:1 Changed 3 years ago by Kwankyu Lee

Branch: u/klee/29994

comment:2 Changed 3 years ago by git

Commit: f3a37b6424f4dd24b4ff17e7be3d1bf25ade4a2d

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

f3a37b6Add integral curves over QQ

comment:3 Changed 3 years ago by Kwankyu Lee

Authors: Kwankyu Lee
Description: modified (diff)
Status: newneeds_review

comment:4 Changed 3 years ago by Matthias Köppe

Thanks for working on this.

There's a duplicated doctest for parametric_representation.

+            sage: p.closed_point()
+            Point (x, y - 1)
+            sage: p.closed_point()
+            Point (x, y - 1)

In Curve, adding a comment on the special handling in the added case would be valuable.

             if k in Fields():
+                if k == QQ and A.coordinate_ring().ideal(F).is_prime():
+                    return IntegralAffineCurve(A, F)
                 return AffineCurve_field(A, F)
             return AffineCurve(A, F)

Also, this is repeated 4 times (for affine/projective, plane/general), so perhaps this can be refactored?

There are also some warnings from patchbot plugins.

comment:5 Changed 3 years ago by git

Commit: f3a37b6424f4dd24b4ff17e7be3d1bf25ade4a2dd9a05b78dd6419a4d57daabcc4ee4670edb7bd7f

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

d9a05b7Fixes for pyflakes, coverage, and reviewer comments

comment:6 in reply to:  4 Changed 3 years ago by Kwankyu Lee

Replying to mkoeppe:

There's a duplicated doctest for parametric_representation.

+            sage: p.closed_point()
+            Point (x, y - 1)
+            sage: p.closed_point()
+            Point (x, y - 1)

Removed.

In Curve, adding a comment on the special handling in the added case would be valuable.

             if k in Fields():
+                if k == QQ and A.coordinate_ring().ideal(F).is_prime():
+                    return IntegralAffineCurve(A, F)
                 return AffineCurve_field(A, F)
             return AffineCurve(A, F)

Also, this is repeated 4 times (for affine/projective, plane/general), so perhaps this can be refactored?

I did some refactoring in the constructor, but perhaps not for the case you commented on.

For the special handling(?), I think the code explains itself. I don't see any helpful comment to add...

There are also some warnings from patchbot plugins.

Fixed for patchbot warnings.

comment:7 Changed 3 years ago by Matthias Köppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

comment:8 in reply to:  7 Changed 3 years ago by Kwankyu Lee

Replying to mkoeppe:

Great! Thanks!

comment:9 Changed 3 years ago by Volker Braun

Branch: u/klee/29994d9a05b78dd6419a4d57daabcc4ee4670edb7bd7f
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.