Opened 12 years ago

Closed 12 years ago

Last modified 7 years ago

#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:

Status badges

Description (last modified by chapoton)

Currently in schemes/generic/ambient_space we see

# Derived classes must overload all of the following functions
def _constructor(self):

        sage: from sage.schemes.generic.ambient_space import AmbientSpace
        sage: A = AmbientSpace(5, ZZ)
        sage: A._constructor()
        Traceback (most recent call last):
    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())
        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)

trac_8675_remove_ambient_space_constructor.patch (9.8 KB) - added by novoselt 12 years ago.

Download all attachments as: .zip

Change History (5)

comment:1 Changed 12 years ago by novoselt

  • Status changed from new to needs_review

comment:2 Changed 12 years ago by AlexGhitza

  • Reviewers set to Alex Ghitza
  • Status changed from needs_review to positive_review

Looks good, passes tests, and fixes a number of inconsistencies.

comment:3 Changed 12 years ago by mhansen

  • 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 chapoton

  • Description modified (diff)
Note: See TracTickets for help on using tickets.