Opened 10 years ago

Closed 10 years ago

#5765 closed defect (fixed)

[with patch, with positive review] improve doctest coverage for schemes/generic/algebraic_scheme.py

Reported by: AlexGhitza Owned by: AlexGhitza
Priority: major Milestone: sage-3.4.2
Component: algebraic geometry Keywords: doctest algebraic scheme
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description


Attachments (4)

trac_5765.patch (27.7 KB) - added by AlexGhitza 10 years ago.
trac_5765-1.patch (30.4 KB) - added by cremona 10 years ago.
Replaces earlier patch
trac_5765-2.patch (1.6 KB) - added by AlexGhitza 10 years ago.
apply after trac_5765-1.patch
trac_5765-rebased.patch (31.6 KB) - added by AlexGhitza 10 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 10 years ago by AlexGhitza

  • Status changed from new to assigned

comment:2 Changed 10 years ago by mabshoff

Alex, is there a patch coming here? :)

Cheers,

Michael

comment:3 Changed 10 years ago by AlexGhitza

Good question. I have done almost all I wanted to do with it, but unfortunately it's all mixed up with another patch that already got merged. It will take me a bit of time to disentangle it, though. Don't wait on it, although I'll do my best to get it done soon.

Changed 10 years ago by AlexGhitza

comment:4 Changed 10 years ago by AlexGhitza

  • Summary changed from improve doctest coverage for schemes/generic/algebraic_scheme.py to [with patch, needs review] improve doctest coverage for schemes/generic/algebraic_scheme.py

The attached patch brings up the doctest coverage from 24% to 87% (29 of 33).

There is also some fairly straightforward refactoring of code, e.g. moving _validate to the ambient spaces where it belongs logically. I also realised that self.complement(other) is counterintuitive and changed it to match normal speech patterns: it should be "the complement of self in other", which is other-self, not self-other. An added bonus is that now one can write X.complement() to get the complement of X in its ambient space, which is highly intuitive. Normally such a change in behaviour would be tricky but since the functions were just broken until 3.4.1.rc3, this shouldn't lead to any confusion (since nobody has used them yet).

Note that the patch relies on changes that only went in at 3.4.1.rc3, so it should only be applied on top of 3.4.1.rc3.

comment:5 Changed 10 years ago by cremona

  • Summary changed from [with patch, needs review] improve doctest coverage for schemes/generic/algebraic_scheme.py to [with patch, with positive review] improve doctest coverage for schemes/generic/algebraic_scheme.py

Patch applies fine and looks good. All doctests in schemes/generic pass. Reference manual docs build ok and look good. Pass!

comment:6 follow-up: Changed 10 years ago by mabshoff

  • Summary changed from [with patch, with positive review] improve doctest coverage for schemes/generic/algebraic_scheme.py to [with patch, needs work] improve doctest coverage for schemes/generic/algebraic_scheme.py

There are some doctest failures against 3.4.1:

        sage -t  devel/sage/sage/schemes/elliptic_curves/ell_point.py # 2 doctests failed
        sage -t  devel/sage/sage/schemes/elliptic_curves/sha_tate.py # 0 doctests failed
        sage -t  devel/sage/sage/schemes/elliptic_curves/ell_generic.py # 3 doctests failed
        sage -t  devel/sage/sage/schemes/plane_curves/constructor.py # 1 doctests failed
        sage -t  devel/sage/sage/schemes/elliptic_curves/ell_field.py # 1 doctests failed

Cheers,

Michael

comment:7 in reply to: ↑ 6 Changed 10 years ago by cremona

Replying to mabshoff:

There are some doctest failures against 3.4.1:

        sage -t  devel/sage/sage/schemes/elliptic_curves/ell_point.py # 2 doctests failed
        sage -t  devel/sage/sage/schemes/elliptic_curves/sha_tate.py # 0 doctests failed
        sage -t  devel/sage/sage/schemes/elliptic_curves/ell_generic.py # 3 doctests failed
        sage -t  devel/sage/sage/schemes/plane_curves/constructor.py # 1 doctests failed
        sage -t  devel/sage/sage/schemes/elliptic_curves/ell_field.py # 1 doctests failed

Cheers,

Michael

Those failures are in files where I changed the docstrings recently, so I'll take a look. John

Changed 10 years ago by cremona

Replaces earlier patch

comment:8 Changed 10 years ago by cremona

  • Summary changed from [with patch, needs work] improve doctest coverage for schemes/generic/algebraic_scheme.py to [with new patch, needs review] improve doctest coverage for schemes/generic/algebraic_scheme.py

The new patch replaces the first one and deals with this. (Sorry, I forgot to do "sage -hg qnew" so it is not a separate patch,

The problem was: in ell_field.py the function _check_satisfies_equations called _error_bad_coords which Alex had renamed. But all it did was riase an error with an appropriate message, so I just raised the error in place.

One remaining question: why is the error raised a TypeError?? In my view the type is fine but the values are wrong if the coordinates do not satisfy the equation. I would have made it a ValueError?. but for consistence with the general scheme functions I left it as a TypeError?.

I put this back to "needs review" but if Michael is happy with test passing there's no more needed.

John

comment:9 follow-up: Changed 10 years ago by AlexGhitza

  • Milestone changed from sage-4.0 to sage-3.4.2
  • Summary changed from [with new patch, needs review] improve doctest coverage for schemes/generic/algebraic_scheme.py to [with patch, positive review] improve doctest coverage for schemes/generic/algebraic_scheme.py

John's changes look good and fix most of the broken doctests.

There was one left, in schemes/plane_curves/constructor.py. The fix is trivial and in the new patch trac_5765-2.patch, which should be applied after trac_5765-1.patch.

Changed 10 years ago by AlexGhitza

apply after trac_5765-1.patch

comment:10 in reply to: ↑ 9 Changed 10 years ago by cremona

Replying to AlexGhitza:

John's changes look good and fix most of the broken doctests.

There was one left, in schemes/plane_curves/constructor.py. The fix is trivial and in the new patch trac_5765-2.patch, which should be applied after trac_5765-1.patch.

Sorry I missed that one!

comment:11 Changed 10 years ago by mabshoff

Ahh, this now collides with #5851 it seems:

sage-3.4.2.alpha0/devel/sage$ patch -p1 --dry-run < trac_5765-1.patch 
patching file sage/schemes/elliptic_curves/ell_field.py
Hunk #1 FAILED at 27.
1 out of 1 hunk FAILED -- saving rejects to file sage/schemes/elliptic_curves/ell_field.py.rej
patching file sage/schemes/elliptic_curves/ell_generic.py
patching file sage/schemes/elliptic_curves/ell_point.py
patching file sage/schemes/generic/affine_space.py
patching file sage/schemes/generic/algebraic_scheme.py
patching file sage/schemes/generic/projective_space.py
patching file sage/schemes/generic/scheme.py

I will see if I can resolve the problem myself.

Cheers,

Michael

comment:12 Changed 10 years ago by mabshoff

  • Summary changed from [with patch, positive review] improve doctest coverage for schemes/generic/algebraic_scheme.py to [with patch, positive review, needs rebase] improve doctest coverage for schemes/generic/algebraic_scheme.py

Ok, looking at the changes I think either John or Alex will rebase this - I am on to other merges for now. So "needs rebase".

Cheers,

Michael

comment:13 Changed 10 years ago by AlexGhitza

I'll do this in a few hours.

comment:14 Changed 10 years ago by AlexGhitza

  • Summary changed from [with patch, positive review, needs rebase] improve doctest coverage for schemes/generic/algebraic_scheme.py to [with patch, needs review] improve doctest coverage for schemes/generic/algebraic_scheme.py

I was working on rebasing the patch against 3.4.2.alpha0, when I realised that ell_field.py has no reason to implement _check_satisfies_equations, since I have fixed the corresponding function "upstream" in algebraic_scheme.py. In other words, the inherited method now works as it should so there's no need to reimplement it for elliptic curves.

Of course, once you remove _check_satisfies_equations from ell_field.py, there's nothing left. I think we just just get rid of this file and make classes that currently inherit from EllipticCurve_field inherit directly from EllipticCurve_generic instead.

The newly attached patch trac_5765-rebased.patch implements all this, and is based on 3.4.2.alpha0. Doctests pass for me, but this is a significant change so I'd like John to review this so he can object if need be.

comment:15 follow-up: Changed 10 years ago by cremona

While I am waiting for 3.4.2.alpha0 to build (before which I cannot test the new patch!) I want to raise an issue.

I agree that the one and only function in ell_field was redundant, since the general machinery for checking that a point lies on a scheme can do what it does. So I am happy with that.

But do we really want to eliminate the *class* EllipticCurve_field? The design seems to be as follows. In principle one can define elliptic curves over general base schemes. The most important special case is elliptic curves over a field. At the moment Sage has essentially no support for an elliptic curve over a non-field, but that might change. So at present, all the generic stuff in ell_generic is quite likely to only work for curves over fields anyway, and that is where anything relevant to general fields is put. Might it not be a better plan to look carefully through ell_generic, see if there is anything there which should really only be for curves over fields, and move that to ell_field (keeping the EllipticCurve_field class)?

Just a thought -- at least a move such as the one this patch does deserves a little more public debate ;)

comment:16 in reply to: ↑ 15 Changed 10 years ago by AlexGhitza

  • Summary changed from [with patch, needs review] improve doctest coverage for schemes/generic/algebraic_scheme.py to [with patch, needs work] improve doctest coverage for schemes/generic/algebraic_scheme.py

Replying to cremona:

While I am waiting for 3.4.2.alpha0 to build (before which I cannot test the new patch!) I want to raise an issue.

I agree that the one and only function in ell_field was redundant, since the general machinery for checking that a point lies on a scheme can do what it does. So I am happy with that.

But do we really want to eliminate the *class* EllipticCurve_field? The design seems to be as follows. In principle one can define elliptic curves over general base schemes. The most important special case is elliptic curves over a field. At the moment Sage has essentially no support for an elliptic curve over a non-field, but that might change. So at present, all the generic stuff in ell_generic is quite likely to only work for curves over fields anyway, and that is where anything relevant to general fields is put. Might it not be a better plan to look carefully through ell_generic, see if there is anything there which should really only be for curves over fields, and move that to ell_field (keeping the EllipticCurve_field class)?

Just a thought -- at least a move such as the one this patch does deserves a little more public debate ;)

Very good, that's why I wanted to hear others say something about it.

I don't feel strongly either way. From what I can see, EllipticCurve_generic has stuff that makes sense for elliptic curves over rings. I agree that it would be good to sift through that and see if there are methods that really only work over fields, and move these to ell_field.py.

In the meantime, I'll modify the patch so that it just removes _check_satisfies_equations and leaves only pass in the definition of the class EllipticCurve_field.

I really need sleep now, but I'll have a corrected patch up in the morning.

comment:17 Changed 10 years ago by was

I think it would be nice to be able to implement the elliptic curve factorization method (ECM) without having to use this hack:

R = Zmod(N)
R.is_field = lambda: True
E = EllipticCurve(R, [-1,0])

Changed 10 years ago by AlexGhitza

comment:18 Changed 10 years ago by AlexGhitza

  • Summary changed from [with patch, needs work] improve doctest coverage for schemes/generic/algebraic_scheme.py to [with patch, needs review] improve doctest coverage for schemes/generic/algebraic_scheme.py

OK, I replaced the controversial version of the patch with the tame one. It is just a rebase of the original positive-reviewed patch, together with the removal of the redundant method _check_satisfies_equations, but leaving a blank ell_field.py for further refactoring as discussed above.

I'm not sure whether this needs a review any more, but if John's around and can have a quick peak, that wouldn't hurt. (Note that the changes to the elliptic curves code are all at the top of the patch.)

comment:19 Changed 10 years ago by AlexGhitza

There's a followup ticket at #5890.

comment:20 Changed 10 years ago by cremona

  • Summary changed from [with patch, needs review] improve doctest coverage for schemes/generic/algebraic_scheme.py to [with patch, with review, needs work] improve doctest coverage for schemes/generic/algebraic_scheme.py

The patch applies but tests in ell_point.py fila revealing this horror:

sage: Fx.<b>=GF(2^(4*5))
sage: Ex=EllipticCurve(Fx,[0,0,1,1,1])
sage: Ex.defining_polynomial()
x^3 + y^2*z + 0*x*z^2 + 0*y*z^2 + 0*z^3

-- note the zero coefficients!

Adding a whole lot of print statements I get to this:

sage: Ex=EllipticCurve(Fx,[0,0,1,1,1])
K =  Finite Field in b of size 2^20
PP =  Projective Space of dimension 2 over Finite Field in b of size 2^20
PP.coordinate_ring() =  Multivariate Polynomial Ring in x, y, z over Finite Field in b of size 2^20
PP.coordinate_ring().base_ring() =  Finite Field in b of size 2^20
PP.coordinate_ring()(1) =  1
ainvs =  [0, 0, 1, 1, 1]
a1, a2, a3, a4, a6 =  0 0 1 1 1
a4*x*z**2 =  0*x*z^2
a4's parent =  Finite Field in b of size 2^20
 f =  x^3 + y^2*z + 0*x*z^2 + 0*y*z^2 + 0*z^3

At which point I am stuck. We have the constant 1 in the multivariate poly ring in x,y,z over the field GF(220), but when I multiply a monomial by it it displays with coefficient zero.

This has surely been caused by something not in this patch! I tried constructing an example outside this bit of code but failed (to make it fail in this way). In fact this happens in unpatched 3.4.2.alpha0:

sage: Fx.<b>=GF(2^(4*5))
sage: Ex=EllipticCurve(Fx,[0,0,1,1,1])
sage: Ex.defining_polynomial()
x^3 + y^2*z + 0*x*z^2 + 0*y*z^2 + 0*z^3

comment:21 Changed 10 years ago by cremona

  • Summary changed from [with patch, with review, needs work] improve doctest coverage for schemes/generic/algebraic_scheme.py to [with patch, with positive review] improve doctest coverage for schemes/generic/algebraic_scheme.py

After applying also the patch at #5919, all tests in sage/schemes pass, so I am changing this to "positive review" but note that it does depend on that patch for the tests in ell_point.py to pass.

The patches (here and at #5919) can be merged in either order.

comment:22 Changed 10 years ago by mabshoff

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

Merged trac_5765-rebased.patch in Sage 3.4.2.rc0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.