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)
Change History (10)
comment:1 Changed 13 years ago by
- Cc ncalexan added
comment:2 Changed 13 years ago by
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
comment:3 Changed 13 years ago by
- 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
- 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?.
comment:5 Changed 13 years ago by
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
- 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
- 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
- Resolution set to fixed
- Status changed from assigned to closed
Merged both patches in Sage 2.10.4.rc0
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