Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#5631 closed enhancement (fixed)

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

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

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 AlexGhitza 11 years ago.
trac_5631_rebased.patch (9.5 KB) - added by AlexGhitza 11 years ago.
apply instead of the previous patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by AlexGhitza

  • Milestone set to sage-3.4.1
  • Summary changed from improve doctest coverage for schemes/generic/affine_space.py to [with patch, needs review] improve doctest coverage for schemes/generic/affine_space.py

comment:2 Changed 11 years ago by AlexGhitza

  • Owner changed from was to AlexGhitza

comment:3 Changed 11 years ago by AlexGhitza

  • Status changed from new to assigned

comment:4 Changed 11 years ago by was

  • Summary changed from [with patch, needs review] improve doctest coverage for schemes/generic/affine_space.py to [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 follow-up: Changed 11 years ago by AlexGhitza

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 11 years ago by was

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 11 years ago by AlexGhitza

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 11 years ago by AlexGhitza

comment:8 Changed 11 years ago by AlexGhitza

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

comment:9 Changed 11 years ago by was

  • Summary changed from [with patch, needs review] improve doctest coverage for schemes/generic/affine_space.py to [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 11 years ago by AlexGhitza

apply instead of the previous patch

comment:10 Changed 11 years ago by AlexGhitza

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

The new patch applies to 3.4.1.rc1.

comment:11 Changed 11 years ago by AlexGhitza

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

comment:12 Changed 11 years ago by was

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

comment:13 Changed 11 years ago by mabshoff

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

Merged trac_5631_rebased.patch in Sage 3.4.1.rc3.

Cheers,

Michael

comment:14 Changed 11 years ago by mabshoff

  • Milestone changed from sage-3.4.2 to sage-3.4.1
Note: See TracTickets for help on using tickets.