#8675 closed defect (fixed)
Remove AmbientSpace._constructor and fix consequences
Reported by: | novoselt | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | major | Milestone: | sage-4.4.4 |
Component: | algebraic geometry | Keywords: | |
Cc: | Merged in: | sage-4.4.4.alpha0 | |
Authors: | Andrey Novoseltsev | Reviewers: | Alex Ghitza |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Currently in schemes/generic/ambient_space we see
... # Derived classes must overload all of the following functions ... def _constructor(self): """ TEST:: sage: from sage.schemes.generic.ambient_space import AmbientSpace sage: A = AmbientSpace(5, ZZ) sage: A._constructor() Traceback (most recent call last): ... NotImplementedError """ raise NotImplementedError ... # End overloads ... def base_extend(self, S, check=True): """ ... """ if is_CommutativeRing(S): R = self.base_ring() if S == R: return self if check: if not S.has_coerce_map_from(R): raise ValueError, "No natural map from the base ring (=%s) to S (=%s)"%(R, S) return self._constructor(self.__n, S, self.variable_names()) else: raise NotImplementedError ...
I have the following problems with it:
- _constructor function has no documentation and a very strange name (because __init__ IS a constructor, why do we need another one?)
- Its usage in the same class calls it with arguments different from specified.
- With these arguments _constructor still would not quite make sense for toric varieties, where dimension and base ring are not sufficient for creation (and if we do take extra information from self, why do we need to pass dimension explicitly?)
- Digging further, I have found the following as a consequence of using _constructor:
sage: A = AffineSpace(2) sage: (A^2).variable_names() ('x0', 'x1', 'x0', 'x1')
I propose the following solution, making AmbientSpace behave closer to FreeModule's like ZZn:
- Remove _constructor
- Make base_extend check if extension is possible and call change_ring
- Make change_ring mandatory for overriding - it should create "exactly the same" ambient space but with a new ring, even if it is not a base extension.
- Powers of affine ambient spaces use default indexed variables rather than trying to cook up something from variables of the base:
sage: A = AffineSpace(2) sage: (A^2).variable_names() ('x0', 'x1', 'x2', 'x3')
Attachments (1)
Change History (5)
Changed 12 years ago by
comment:1 Changed 12 years ago by
- Status changed from new to needs_review
comment:2 Changed 12 years ago by
- Reviewers set to Alex Ghitza
- Status changed from needs_review to positive_review
comment:3 Changed 12 years ago by
- Merged in set to sage-4.4.4.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:4 Changed 7 years ago by
- Description modified (diff)
Note: See
TracTickets for help on using
tickets.
Looks good, passes tests, and fixes a number of inconsistencies.