Opened 12 years ago
Closed 7 years ago
#7946 closed defect (fixed)
Fix TestSuite failures for schemes
Reported by: | nthiery | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | major | Milestone: | sage-6.4 |
Component: | algebraic geometry | Keywords: | |
Cc: | vbraun | Merged in: | |
Authors: | Peter Bruin | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 5fd92ee (Commits, GitHub, GitLab) | Commit: | 5fd92eed3024163fa068bea8e3644d677f151adc |
Dependencies: | #16158 | Stopgaps: |
Description (last modified by )
Consider the following situation:
sage: S = Spec(ZZ) sage: x = S.an_element()
Running TestSuite(S)
and TestSuite(x)
yields several failures. A related problem is
sage: S Spectrum of Integer Ring sage: parent(x) Set of rational points of Spectrum of Integer Ring
whereas we expect parent(x) is S
.
Here is the complete TestSuite
report:
sage: TestSuite(S).run() Failure in _test_an_element: Traceback (most recent call last): ... NotImplementedError ------------------------------------------------------------ Failure in _test_category: Traceback (most recent call last): ... AssertionError: False is not true ------------------------------------------------------------ Failure in _test_pickling: Traceback (most recent call last): ... AssertionError: Point on Spectrum of Integer Ring defined by the Principal ideal (991) of Integer Ring != Point on Spectrum of Integer Ring defined by the Principal ideal (991) of Integer Ring ------------------------------------------------------------ The following tests failed: _test_category, _test_pickling Failure in _test_elements Failure in _test_some_elements: Traceback (most recent call last): ... NotImplementedError ------------------------------------------------------------ The following tests failed: _test_an_element, _test_elements, _test_some_elements
sage: TestSuite(x).run() Failure in _test_category: Traceback (most recent call last): ... AssertionError: False is not true ------------------------------------------------------------ Failure in _test_pickling: Traceback (most recent call last): ... AssertionError: Point on Spectrum of Integer Ring defined by the Principal ideal (991) of Integer Ring != Point on Spectrum of Integer Ring defined by the Principal ideal (991) of Integer Ring ------------------------------------------------------------ The following tests failed: _test_category, _test_pickling
(Note: the NotImplementedError
that one gets after applying #16158 is a different one than before.)
Change History (14)
comment:1 Changed 12 years ago by
- Description modified (diff)
comment:2 Changed 12 years ago by
- Milestone set to sage-4.3.2
comment:3 Changed 10 years ago by
- Cc vbraun added
comment:4 Changed 8 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:5 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:6 Changed 8 years ago by
What is Spec
intended to mean in the first place? The definition currently starts with
class Spec(AffineScheme): ....
This suggests that Spec
is a special kind of affine scheme. But aren't "the spectrum of a ring" and "an affine scheme" exactly the same thing?
This code probably predates the category framework. A more modern design would perhaps be something like the following:
- define the category
AffineSchemes
(orSchemes().Affine()
) of affine schemes; - designate
AffineScheme
as the "canonical" type for objects in this category, and move functionality fromSpec
toAffineScheme
as appropriate; - convert
Spec
into a functor fromCommutativeRings
toAffineSchemes
.
(Mathematically speaking, Spec [as a functor from commutative rings to schemes, or even locally ringed spaces] can be defined as the adjoint functor of the global sections functor X -> O_{X}(X) from schemes to commutative rings, and this gives an anti-equivalence of categories from commutative rings to affine schemes; I wonder if this could somehow be formalised in Sage's category framework, but that's another story...)
comment:7 Changed 7 years ago by
- Dependencies set to #16158
- Description modified (diff)
- Summary changed from Spec(...) does not specify its category to Fix TestSuite failures for schemes
To show that the first part of the original ticket is fixed:
sage: S = Spec(ZZ) sage: S.category() Category of Schemes sage: isinstance(S, S.category().parent_class) True
comment:8 follow-up: ↓ 11 Changed 7 years ago by
- Branch set to u/pbruin/7946-scheme_point_improvements
- Commit set to 5fd92eed3024163fa068bea8e3644d677f151adc
- Description modified (diff)
- Status changed from new to needs_review
The attached branch makes affine schemes, and points on them, cooperate better with the category and coercion code. The TestSuite
now runs without failures.
comment:9 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:10 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:11 in reply to: ↑ 8 ; follow-up: ↓ 12 Changed 7 years ago by
- Reviewers set to Travis Scrimshaw
Replying to pbruin:
The attached branch makes affine schemes, and points on them, cooperate better with the category and coercion code. The
TestSuite
now runs without failures.
Is there some way you can get rid of that custom __call__
method in AffineScheme
, or at least passes things up through the Parent.__call__
when trying to construct an element? If not, then I think we're okay setting this to positive review as it fixes the issues as stated in the ticket description (you can do this on my behalf).
Also, I think that comment:6 (from the very limited understanding I have of the math) is the correct way to do things. However that would be a major overhaul to be done on a separate ticket, and perhaps the above change to __call__
would fit into that.
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 13 Changed 7 years ago by
- Status changed from needs_review to positive_review
Replying to tscrim:
Replying to pbruin:
The attached branch makes affine schemes, and points on them, cooperate better with the category and coercion code. The
TestSuite
now runs without failures.Is there some way you can get rid of that custom
__call__
method inAffineScheme
, or at least passes things up through theParent.__call__
when trying to construct an element? If not, then I think we're okay setting this to positive review as it fixes the issues as stated in the ticket description (you can do this on my behalf).
The reason this is done via the __call__()
method is that an affine scheme S accepts two kinds of input; only one of them (input is a prime ideal) constructs an element with S as its parent. In the other case (input is a list of coordinates) the __call__()
syntax constructs an element of a suitable scheme homset. I don't see an easy way to avoid a custom __call__()
method; it may be possible (and would be nice) to get rid of it, but it is better to do this on separate ticket. (Note also that the __call__()
method was already present, I just improved it as necessary.)
Also, I think that comment:6 (from the very limited understanding I have of the math) is the correct way to do things. However that would be a major overhaul to be done on a separate ticket, and perhaps the above change to
__call__
would fit into that.
What I was talking about in that comment was mostly done on #16158 (closed a couple of months ago). I think the remaining issue of making a category AffineSchemes
would probably be disjoint from getting rid of the __call__()
method.
I'm setting this to positive review since you gave the green light.
comment:13 in reply to: ↑ 12 Changed 7 years ago by
The reason this is done via the
__call__()
method is that an affine scheme S accepts two kinds of input; only one of them (input is a prime ideal) constructs an element with S as its parent. In the other case (input is a list of coordinates) the__call__()
syntax constructs an element of a suitable scheme homset. I don't see an easy way to avoid a custom__call__()
method; it may be possible (and would be nice) to get rid of it, but it is better to do this on separate ticket. (Note also that the__call__()
method was already present, I just improved it as necessary.)
That's what I was thinking from a quick look through. Anyways, for later.
What I was talking about in that comment was mostly done on #16158 (closed a couple of months ago). I think the remaining issue of making a category
AffineSchemes
would probably be disjoint from getting rid of the__call__()
method.
I knew the last one was done, but forgot that it moved the functionality. Unfortunately I think you're right, but we can at least do a followup to create the category of AffineSchemes
.
I'm setting this to positive review since you gave the green light.
Thanks.
comment:14 Changed 7 years ago by
- Branch changed from u/pbruin/7946-scheme_point_improvements to 5fd92eed3024163fa068bea8e3644d677f151adc
- Resolution set to fixed
- Status changed from positive_review to closed
#11599 claims to fix this, but I am not entirely sure what would it take to fix this ticket - the reported category is now schemes and only 3 of the
TestSuite
tests fail.