Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#5631 closed enhancement (fixed)

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

Reported by: Alex Ghitza Owned by: Alex Ghitza
Priority: minor Milestone: sage-3.4.1
Component: algebraic geometry Keywords: doctest affine space
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

The attached patch improves the doctest coverage of affine_space.py from 45% (9 of 20) to 80% (16 of 20) and fixes a few tiny bugs along the way.

Attachments (2)

trac_5631.patch (9.5 KB) - added by Alex Ghitza 14 years ago.
trac_5631_rebased.patch (9.5 KB) - added by Alex Ghitza 14 years ago.
apply instead of the previous patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 14 years ago by Alex Ghitza

Milestone: sage-3.4.1
Summary: improve doctest coverage for schemes/generic/affine_space.py[with patch, needs review] improve doctest coverage for schemes/generic/affine_space.py

comment:2 Changed 14 years ago by Alex Ghitza

Owner: changed from William Stein to Alex Ghitza

comment:3 Changed 14 years ago by Alex Ghitza

Status: newassigned

comment:4 Changed 14 years ago by William Stein

Summary: [with patch, needs review] improve doctest coverage for schemes/generic/affine_space.py[with patch, needs work] improve doctest coverage for schemes/generic/affine_space.py

I don't understand why you deleted lines 415, 416 below:

414	505	    def subscheme_complement(self, X, Y): 
415	 	        X = self.subscheme(X) 
416	 	        Y = self.subscheme(Y) 

The way you have the code now, it subscheme_complement has nothing at all to do with self. Why is it even a method of self as is now.

Otherwise this patch looks very good.

comment:5 Changed 14 years ago by Alex Ghitza

I had two problems with lines 415 and 416: (1) the doctest that I wrote wasn't working and (2) they aren't there in the corresponding method in projective_space.py. If you look at the docstring that I wrote for subscheme_complement (and the doctest), you will notice that X and Y are already subschemes of self (so self is involved in a slightly hidden way).

I guess that one could relax the syntax so that polynomial lists can be passed to subscheme_complement; in the example of the doctest this would be

sage: A.<x, y, z> = AffineSpace(3, ZZ)
sage: A.subscheme_complement([x+y-z], [x-y+z])

Is this what you have in mind?

comment:6 in reply to:  5 Changed 14 years ago by William Stein

If you look at the docstring that I wrote for subscheme_complement (and the doctest), you will notice that X and Y are already subschemes of self (so self is involved in a slightly hidden way).

Couldn't I do this:

sage: A.<x, y, z> = AffineSpace(3, ZZ)
sage: [[make an X and a Y over GF(7) say that haven't nothing to do with A]]
sage: A.subscheme_complement(X, Y)

Either subscheme_complement needs to check that X and Y are really subschemes of A, or it should just be a method of X, i.e.,

sage: X.complement(Y)

say, which requires that X and Y live in a common ambient space.

-- William

comment:7 Changed 14 years ago by Alex Ghitza

So I looked at algebraic_scheme.py and noticed that there is a method X.exclude(Y) which seems to want to do what you're suggesting. It's actually broken as it is, and I doubt that it ever worked (no doctests, not used anywhere in the Sage library, and the simplest tests that I thought of didn't work). So I'm renaming it to X.complement(Y) and removing subscheme_complement() from both affine_space.py and projective_space.py. They are also not used anywhere.

Changed 14 years ago by Alex Ghitza

Attachment: trac_5631.patch added

comment:8 Changed 14 years ago by Alex Ghitza

Summary: [with patch, needs work] improve doctest coverage for schemes/generic/affine_space.py[with patch, needs review] improve doctest coverage for schemes/generic/affine_space.py

comment:9 Changed 14 years ago by William Stein

Summary: [with patch, needs review] improve doctest coverage for schemes/generic/affine_space.py[with patch, needs work] improve doctest coverage for schemes/generic/affine_space.py

Can you rebase it? Or does it depends on something? I can't apply it:

teragon:build wstein$ sage
----------------------------------------------------------------------
| Sage Version 3.4.1.rc1, Release Date: 2009-04-05                   |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
hg_sage.apply('http://trac.sagemath.org/sage_trac/raw-attachment/ticket/5631/trac_5631.patch')
sage: hg_sage.apply('http://trac.sagemath.org/sage_trac/raw-attachment/ticket/5631/trac_5631.patch')
Attempting to load remote file: http://trac.sagemath.org/sage_trac/raw-attachment/ticket/5631/trac_5631.patch
Loading: [..]
cd "/Users/wstein/build/sage-3.4.1.rc1/devel/sage" && hg status
cd "/Users/wstein/build/sage-3.4.1.rc1/devel/sage" && hg status
cd "/Users/wstein/build/sage-3.4.1.rc1/devel/sage" && hg import   "/Users/wstein/.sage/temp/teragon.local/86960/tmp_1.patch"
applying /Users/wstein/.sage/temp/teragon.local/86960/tmp_1.patch
patching file sage/schemes/generic/affine_space.py
Hunk #4 FAILED at 208
Hunk #5 succeeded at 243 with fuzz 2 (offset 0 lines).
Hunk #6 FAILED at 264
2 out of 8 hunks FAILED -- saving rejects to file sage/schemes/generic/affine_space.py.rej
abort: patch failed to apply
sage: 

Changed 14 years ago by Alex Ghitza

Attachment: trac_5631_rebased.patch added

apply instead of the previous patch

comment:10 Changed 14 years ago by Alex Ghitza

Yes, there was some interaction with the ticket that fixed dimension issues.

The new patch applies to 3.4.1.rc1.

comment:11 Changed 14 years ago by Alex Ghitza

Summary: [with patch, needs work] improve doctest coverage for schemes/generic/affine_space.py[with patch, needs review] improve doctest coverage for schemes/generic/affine_space.py

comment:12 Changed 14 years ago by William Stein

Summary: [with patch, needs review] improve doctest coverage for schemes/generic/affine_space.py[with patch, positive review] improve doctest coverage for schemes/generic/affine_space.py

comment:13 Changed 14 years ago by Michael Abshoff

Resolution: fixed
Status: assignedclosed

Merged trac_5631_rebased.patch in Sage 3.4.1.rc3.

Cheers,

Michael

comment:14 Changed 14 years ago by Michael Abshoff

Milestone: sage-3.4.2sage-3.4.1
Note: See TracTickets for help on using tickets.