Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove AmbientSpace._constructor and fix consequences #8675

Closed
novoselt opened this issue Apr 11, 2010 · 5 comments
Closed

Remove AmbientSpace._constructor and fix consequences #8675

novoselt opened this issue Apr 11, 2010 · 5 comments

Comments

@novoselt
Copy link
Member

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 ZZ^n:

  • 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')

Component: algebraic geometry

Author: Andrey Novoseltsev

Reviewer: Alex Ghitza

Merged: sage-4.4.4.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/8675

@novoselt
Copy link
Member Author

comment:1

Attachment: trac_8675_remove_ambient_space_constructor.patch.gz

@aghitza
Copy link
Contributor

aghitza commented May 18, 2010

Reviewer: Alex Ghitza

@aghitza
Copy link
Contributor

aghitza commented May 18, 2010

comment:2

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

@mwhansen
Copy link
Contributor

mwhansen commented Jun 6, 2010

Merged: sage-4.4.4.alpha0

@fchapoton

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants