Opened 17 months ago
Closed 17 months ago
#20697 closed enhancement (fixed)
Inheritance structure of generic projective/affine curves
Reported by:  gjorgenson  Owned by:  

Priority:  minor  Milestone:  sage7.3 
Component:  algebraic geometry  Keywords:  gsoc2016 
Cc:  mmarco, bhutz  Merged in:  
Authors:  Grayson Jorgenson  Reviewers:  Ben Hutz 
Report Upstream:  N/A  Work issues:  
Branch:  4ac79ab (Commits)  Commit:  4ac79ab4499d76428dd416a21d9fc11639e72a29 
Dependencies:  Stopgaps: 
Description
Currently there exist classes for generic projective/affine space and plane curves, but the plane curve classes do not inherit from the respective space curve classes. Adding this inheritance will facilitate the implementation of future generic curve functionality.
Change History (26)
comment:1 Changed 17 months ago by
 Branch set to u/gjorgenson/ticket/20697
comment:2 Changed 17 months ago by
 Commit set to 5de70c9c1041a9aea0d4edbb66c43d82ce5bf4ef
comment:3 Changed 17 months ago by
 Status changed from new to needs_review
comment:4 followup: ↓ 5 Changed 17 months ago by
 Status changed from needs_review to needs_info
I do not see anything wrong with the new inheritance structure, but I don't really understand the naming conventions of the classes. Here is what I see
AffineSpaceCurve?, ProjectiveSpaceCurve? = the generic curves classes
AffineCurve_generic, ProjectiveCurve_generic  the curves that must lie in the plane (A2 or P2).
That seems somewhat odd to me. If I've understood this correctly doesn't some naming convention like
AffineCurve_generic, ProjectiveCurve_generic > XXXXCurve_plane
AffineSpaceCurve?, ProjectiveSpaceCurve? ? XXXXCurve_generic
make more sense. Then the special class inherits from the 'generic' class instead of the other way around.
Also, it seems like some description should be added to the documentation for all of these classes.
comment:5 in reply to: ↑ 4 Changed 17 months ago by
Replying to bhutz:
I think that would be a good renaming. I didn't try to implement a name change earlier because changing the ProjectiveCurve_generic class name would require changing it when referenced in the elliptic curve, conic, and hyperelliptic curve classes. I actually don't think that would be so bad; I'll try changing the names and see how much needs to be done.
Perhaps at some point it would be good to change the folder name as well, since the eventually the generic curve classes will have more functionality for general curves.
comment:6 Changed 17 months ago by
We should discuss this change of names in sagedevel too, since it might affect the public interface (i.e. it could break user's code).
comment:7 Changed 17 months ago by
If we're changing names, I'd rather change them all at once, so include the folder in the post to sagedevel. Perhaps either curves or curves_generic?
comment:8 Changed 17 months ago by
 Commit changed from 5de70c9c1041a9aea0d4edbb66c43d82ce5bf4ef to 5a5fe9a942ea9ba0e14039bcd4d1c9695545f035
comment:9 Changed 17 months ago by
 Commit changed from 5a5fe9a942ea9ba0e14039bcd4d1c9695545f035 to b3e8447d20c03a4c1970133728c49773335dbe36
Branch pushed to git repo; I updated commit sha1. New commits:
b3e8447  20697: Merge with latest beta.

comment:10 Changed 17 months ago by
 Status changed from needs_info to needs_review
Okay, I've made an attempt to change the class names and folder name. I think I've changed all occurrences of the names, and so far everything seems to be working properly.
comment:11 Changed 17 months ago by
 Reviewers set to Ben Hutz
 Status changed from needs_review to needs_work
functionality seems fine, but I'd like to see a description of the classes and some examples in each of the files
 affine_curve
 projective_curve
 curve
For example look at the beginning of the projective_space.py file.
comment:12 Changed 17 months ago by
 Commit changed from b3e8447d20c03a4c1970133728c49773335dbe36 to 85415915be7bf80ae7819e8ecdf3d0b66a2ce157
Branch pushed to git repo; I updated commit sha1. New commits:
9cbdab3  20698: revised initialization of generic curves.

b718afa  20698: documentation and error formatting fixes.

d4eb8d4  20698: documentation spacing fixes.

3b5fdf8  20697: Merge in 20698 for access to better initialization of curves.

8541591  20697: Added documentation and made curve string labels match class names.

comment:13 Changed 17 months ago by
 Status changed from needs_work to needs_review
I added some documentation to the beginning of affine_curve and projective_curve, and to the __init__
functions. The string labels for the curves no longer matched the class names, so I changed that as well (I had to change some examples in a few other files in the scheme folder to do this).
comment:14 Changed 17 months ago by
 Status changed from needs_review to needs_work
minor: notice that it is possible to define curves over general rings since the init just calls the suscheme init. So you should remove the "over fields" part of the class title for projective and affine curves.
from src/sage/structure/sage_object.pyx pickle failure: It looks like you are going to need to do a register_unpickle_override for projective curves.
ImportError: cannot import ProjectiveCurve_generic from sage.schemes.plane_curves.projective_curve, call register_unpickle_override('sage.schemes.plane_curves.projective_curve', 'ProjectiveCurve_generic', ...) to fix this Failed: _class__sage_schemes_jacobians_abstract_jacobian_Jacobian_generic__.sobj  ** This error is probably due to an old pickle failing to unpickle. ** See sage.structure.sage_object.register_unpickle_override for ** how to override the default unpickling methods for (old) pickles. ** NOTE: pickles should never be removed from the pickle_jar!  Successfully unpickled 585 objects. Failed to unpickle 1 objects.
comment:15 Changed 17 months ago by
 Commit changed from 85415915be7bf80ae7819e8ecdf3d0b66a2ce157 to b918e772276ed7ed576e8eae9936eeca9b2671cf
Branch pushed to git repo; I updated commit sha1. New commits:
b918e77  20697: documentation adjustment, and attempt to fix pickle issue.

comment:16 Changed 17 months ago by
 Status changed from needs_work to needs_review
Alright, I followed what was done in affine_space.py
for addressing the pickle issue, and I think it worked.
comment:17 Changed 17 months ago by
 Commit changed from b918e772276ed7ed576e8eae9936eeca9b2671cf to b3809671bae3c03fe2407471a1e908cdbdc22a00
Branch pushed to git repo; I updated commit sha1. New commits:
b380967  20697: Missed name changes.

comment:18 Changed 17 months ago by
Oops, I neglected to do a full doctest until now, and found a few examples I missed in the tutorials that were broken because of the string representation changes for curves. Everything else seems to be okay.
comment:19 Changed 17 months ago by
 Keywords gsoc2016 added
 Status changed from needs_review to positive_review
make ptestlong now passes
comment:20 Changed 17 months ago by
 Status changed from positive_review to needs_work
sage t long src/sage/schemes/projective/projective_morphism.py ********************************************************************** File "src/sage/schemes/projective/projective_morphism.py", line 835, in sage.schemes.projective.projective_morphism.SchemeMorphism_polynomial_projective_space.dynatomic_polynomial Failed example: f.dynatomic_polynomial(2) Exception raised: Traceback (most recent call last): File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 498, in _run self.compile_and_execute(example, compiler, test.globs) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 861, in compile_and_execute exec(compiled, globs) File "<doctest sage.schemes.projective.projective_morphism.SchemeMorphism_polynomial_projective_space.dynatomic_polynomial[69]>", line 1, in <module> f.dynatomic_polynomial(Integer(2)) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/schemes/projective/projective_morphism.py", line 928, in dynatomic_polynomial PHI = PHI.numerator()._maxima_().divide(PHI.denominator())[0].sage() File "sage/structure/sage_object.pyx", line 795, in sage.structure.sage_object.SageObject._maxima_ (/Users/buildslavesage/slave/sage_git/build/src/build/cythonized/sage/structure/sage_object.c:8292) return self._interface_(G) File "sage/structure/sage_object.pyx", line 707, in sage.structure.sage_object.SageObject._interface_ (/Users/buildslavesage/slave/sage_git/build/src/build/cythonized/sage/structure/sage_object.c:5981) X = I(s) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/interfaces/interface.py", line 243, in __call__ return cls(self, x, name=name) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/interfaces/maxima.py", line 1159, in __init__ ExpectElement.__init__(self, parent, value, is_name=False, name=None) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/interfaces/expect.py", line 1376, in __init__ self._name = parent._create(value, name=name) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/interfaces/interface.py", line 433, in _create self.set(name, value) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/interfaces/maxima.py", line 1004, in set self._eval_line(cmd) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/interfaces/maxima.py", line 791, in _eval_line assert line_echo.strip().endswith(line.strip()), 'mismatch:\n' + line_echo + line AssertionError: mismatch: sage11 : x^4*y + (0.666666666666667*I)*x^2*y^3  x*y^4 + (0.111111111111111  0.333333333333333*I)*y^5$
comment:21 Changed 17 months ago by
Could you add some more information about the machine this is failing on. I just tested it again based on 7.3.beta2 Ubuntu 64bit and it passes for me.
comment:22 Changed 17 months ago by
Can you just wait for the next beta and merge it in?
comment:23 Changed 17 months ago by
 Commit changed from b3809671bae3c03fe2407471a1e908cdbdc22a00 to 4ac79ab4499d76428dd416a21d9fc11639e72a29
Branch pushed to git repo; I updated commit sha1. New commits:
4ac79ab  20697: merge this ticket with new beta.

comment:24 Changed 17 months ago by
 Status changed from needs_work to needs_review
Okay, this should now be based on sage 7.3 beta3.
comment:25 Changed 17 months ago by
 Status changed from needs_review to positive_review
all passes for me
comment:26 Changed 17 months ago by
 Branch changed from u/gjorgenson/ticket/20697 to 4ac79ab4499d76428dd416a21d9fc11639e72a29
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
20697: plane curve classes now inherit from the space curve classes.