Opened 13 years ago

Closed 13 years ago

#2485 closed defect (fixed)

[with patches, with positive review] Complete docstrings and doctests for schemes/elliptic_curves

Reported by: cremona Owned by: cremona
Priority: major Milestone: sage-2.10.4
Component: algebraic geometry Keywords: elliptic curves
Cc: ncalexan Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Following Doc Days 2 on 2008-03-09 I am working on making the docstrings and tests for schemes/elliptic_curves as complete as I can. A patch based on 2.10.3 will be posted here in time for 2.10.4, I hope.

Attachments (2)

8866.patch (81.8 KB) - added by cremona 13 years ago.
8867.patch (2.6 KB) - added by cremona 13 years ago.
Apply after 8866.patch

Download all attachments as: .zip

Change History (10)

comment:1 Changed 13 years ago by ncalexan

  • Cc ncalexan added

Hi John,

I assume you're CCed on this. I am actively using the hyperelliptic curve code and have lots of things to fix/upgrade. Some of my work will touch schemes/elliptic_curves and I don't want to step on your toes. I'm CCed on this; when your patch is done I'll gladly referee it.

Nick

comment:2 Changed 13 years ago by cremona

Thanks Nick, that will be very helpful. It's unlikely that we will conflict. There is almost nothing I am planning to do except add to docstrings. John

Changed 13 years ago by cremona

comment:3 Changed 13 years ago by cremona

  • Status changed from new to assigned

The attached patch adds many docstrings to the following files (based on 2.10.3):

  • M sage/schemes/elliptic_curves/ell_finite_field.py
  • M sage/schemes/elliptic_curves/ell_generic.py
  • M sage/schemes/elliptic_curves/ell_modular_symbols.py
  • M sage/schemes/elliptic_curves/ell_number_field.py
  • M sage/schemes/elliptic_curves/ell_padic.py
  • M sage/schemes/elliptic_curves/ell_padic_field.py
  • M sage/schemes/elliptic_curves/ell_point.py
  • M sage/schemes/elliptic_curves/ell_rational_field.py
  • M sage/schemes/elliptic_curves/ell_tate_curve.py
  • M sage/schemes/elliptic_curves/gp_cremona.py
  • M sage/schemes/elliptic_curves/gp_simon.py

In addition there are a very few changes other than docstrings.

  • Some internal and local functions have had _ prepended to their names to explain why they may not have doctests
  • The output type of modular_parametrization() in ell_rational_field.py has been changed from a list of pari types to a list of Sage Laurent Series in the variable q
  • In addition to has_cm for elliptic curves over QQ, there is now a function cm_discriminant(). These two functions refer to a new global (to ell_rational_field.py) dict structure called CMJ.

I hope all this will be found useful! There is still more to do in this directory:

Overall weighted coverage score: 66.1%

but a lot of that is explained by monsky_washnitzer.py: 23% (25 of 107) whci someone else will do, I hope.

comment:4 Changed 13 years ago by ncalexan

  • Summary changed from Complete docstrings and doctests for schemes/elliptic_curves to [with patch, with positive review] Complete docstrings and doctests for schemes/elliptic_curves

I think this patch should be applied, because it is mostly good, but it's not perfect. I've noted a few nits below, only one of which (the KeyError?) should be addressed before application.

Thanks for your effort, John!

This is probably not your bug, John, but it doesn't look right.

606	        EXAMPLES: 
607	            sage: E=EllipticCurve(GF(5),[1,1]) 
608	            sage: E._homset_class(GF(5^10,'a'),GF(5)) 
609	            Abelian group of points on Finite Field in a of size 5^10

Also, I really worry about double underscore functions at all -- I say replace with single underscore; then doctesting isn't so strange.

 	632	            sage: E=EllipticCurve(QQ,[1,1]) 
 	633	            sage: E._EllipticCurve_generic__is_over_RationalField() 
 	634	            True 

This comment looks outdated, and should be removed:

 	521	        def _pval(x):   # cannot be used for x=0 
 	522	            """ 
 	523	            Local function returning the valuation of x at P 
 	524	            """ 
522	525	            if x==0: return Infinity 

I think things like

1809	2264	    def label(self): 
1810	2265	        r""" 
1811	2266	        Exactly the same as the \code{cremona_label()} command. 
 	2267	 
 	2268	        EXAMPLES: 
 	2269	            sage: E=EllipticCurve('389a1') 
 	2270	            sage: E.label() 
 	2271	            '389a1' 
 	2272	 
1812	2273	        """ 
1813	2274	        return self.cremona_label() 

should be replaced with label = cremona_label.

This is not very helpful -- I would prefer, "A ValueError? is raised..."

        2390	        An error is raised if the curve does not have CM (see the 
 	2391	        function has_cm()) 

and change the code to raise ValueError?; it seems more appropriate than KeyError?.

Changed 13 years ago by cremona

Apply after 8866.patch

comment:5 Changed 13 years ago by cremona

Thanks a lot for the fast review, Nick.

  • I left the _homset_class test as it was. That was one where I had a hard time working out what the function did and what was the point of it. I still do not know.
  • Doctesting double-underscore functions is indeed very hard. For things like init() the test does not mention init() and "sage -t" complains. For ones like _EllipticCurve_generic__is_over_RationalField() (where in the code the function is simply __is_over_RationalField() there's no way the user will ever get to use them, and putting in tests will hardly help developers.
  • I fixed the others as you suggested, including changing the KeyError to ValueError -- it's just that the error is first raised as a KeyError (the value of j is not a key in the dict CMJ), but the user need not know that.

comment:6 Changed 13 years ago by cremona

  • Summary changed from [with patch, with positive review] Complete docstrings and doctests for schemes/elliptic_curves to [with patch, with positive review, with new patch, needs further review] Complete docstrings and doctests for schemes/elliptic_curves

comment:7 Changed 13 years ago by AlexGhitza

  • Summary changed from [with patch, with positive review, with new patch, needs further review] Complete docstrings and doctests for schemes/elliptic_curves to [with patches, with positive review] Complete docstrings and doctests for schemes/elliptic_curves

Both patches apply cleanly against 2.10.3, and all doctests pass in sage/schemes/elliptic_curves/

The second patch appears to fix the issues brought up by the first reviewer, so I say "thumbs up".

comment:8 Changed 13 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged both patches in Sage 2.10.4.rc0

Note: See TracTickets for help on using tickets.