#20697 closed enhancement (fixed)

Inheritance structure of generic projective/affine curves

Reported by: gjorgenson Owned by:
Priority: minor Milestone: sage-7.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 12 months ago by gjorgenson

  • Branch set to u/gjorgenson/ticket/20697

comment:2 Changed 12 months ago by git

  • Commit set to 5de70c9c1041a9aea0d4edbb66c43d82ce5bf4ef

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

5de70c920697: plane curve classes now inherit from the space curve classes.

comment:3 Changed 12 months ago by gjorgenson

  • Status changed from new to needs_review

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

  • 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 12 months ago by gjorgenson

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 12 months ago by mmarco

We should discuss this change of names in sage-devel too, since it might affect the public interface (i.e. it could break user's code).

comment:7 Changed 12 months ago by bhutz

If we're changing names, I'd rather change them all at once, so include the folder in the post to sage-devel. Perhaps either curves or curves_generic?

comment:8 Changed 12 months ago by git

  • Commit changed from 5de70c9c1041a9aea0d4edbb66c43d82ce5bf4ef to 5a5fe9a942ea9ba0e14039bcd4d1c9695545f035

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

058b90920697: Change class names.
5a5fe9a20697: change "plane_curves" folder name to "curves"

comment:9 Changed 12 months ago by git

  • Commit changed from 5a5fe9a942ea9ba0e14039bcd4d1c9695545f035 to b3e8447d20c03a4c1970133728c49773335dbe36

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

b3e844720697: Merge with latest beta.

comment:10 Changed 12 months ago by gjorgenson

  • 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 12 months ago by bhutz

  • 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 12 months ago by git

  • Commit changed from b3e8447d20c03a4c1970133728c49773335dbe36 to 85415915be7bf80ae7819e8ecdf3d0b66a2ce157

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

9cbdab320698: revised initialization of generic curves.
b718afa20698: documentation and error formatting fixes.
d4eb8d420698: documentation spacing fixes.
3b5fdf820697: Merge in 20698 for access to better initialization of curves.
854159120697: Added documentation and made curve string labels match class names.

comment:13 Changed 12 months ago by gjorgenson

  • 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 12 months ago by bhutz

  • 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 12 months ago by git

  • Commit changed from 85415915be7bf80ae7819e8ecdf3d0b66a2ce157 to b918e772276ed7ed576e8eae9936eeca9b2671cf

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

b918e7720697: documentation adjustment, and attempt to fix pickle issue.

comment:16 Changed 12 months ago by gjorgenson

  • 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 12 months ago by git

  • Commit changed from b918e772276ed7ed576e8eae9936eeca9b2671cf to b3809671bae3c03fe2407471a1e908cdbdc22a00

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

b38096720697: Missed name changes.

comment:18 Changed 12 months ago by gjorgenson

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 12 months ago by bhutz

  • Keywords gsoc2016 added
  • Status changed from needs_review to positive_review

make ptestlong now passes

comment:20 Changed 12 months ago by vbraun

  • 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/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 498, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/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/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/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/buildslave-sage/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/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/structure/sage_object.c:5981)
        X = I(s)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/interface.py", line 243, in __call__
        return cls(self, x, name=name)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/maxima.py", line 1159, in __init__
        ExpectElement.__init__(self, parent, value, is_name=False, name=None)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 1376, in __init__
        self._name = parent._create(value, name=name)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/interface.py", line 433, in _create
        self.set(name, value)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/maxima.py", line 1004, in set
        self._eval_line(cmd)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/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 12 months ago by bhutz

Could you add some more information about the machine this is failing on. I just tested it again based on 7.3.beta2 Ubuntu 64-bit and it passes for me.

comment:22 Changed 12 months ago by vbraun

Can you just wait for the next beta and merge it in?

comment:23 Changed 12 months ago by git

  • Commit changed from b3809671bae3c03fe2407471a1e908cdbdc22a00 to 4ac79ab4499d76428dd416a21d9fc11639e72a29

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

4ac79ab20697: merge this ticket with new beta.

comment:24 Changed 12 months ago by gjorgenson

  • Status changed from needs_work to needs_review

Okay, this should now be based on sage 7.3 beta3.

comment:25 Changed 12 months ago by bhutz

  • Status changed from needs_review to positive_review

all passes for me

comment:26 Changed 12 months ago by vbraun

  • Branch changed from u/gjorgenson/ticket/20697 to 4ac79ab4499d76428dd416a21d9fc11639e72a29
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.