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

trying to get rid of coerce_c_impl #39419

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/sage/rings/polynomial/multi_polynomial_libsingular.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -499,8 +499,8 @@ cdef class MPolynomialRing_libsingular(MPolynomialRing_base):
if self is other:
return True
n = other.ngens()
if(other.base_ring is base_ring and self.ngens() >= n and
self.variable_names()[:n] == other.variable_names()):
if (other.base_ring is base_ring and self.ngens() >= n and
self.variable_names()[:n] == other.variable_names()):
return True
elif base_ring.has_coerce_map_from(other._mpoly_base_ring(self.variable_names())):
return True
Expand Down
2 changes: 0 additions & 2 deletions src/sage/rings/polynomial/multi_polynomial_ring_base.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ cdef class MPolynomialRing_base(CommutativeRing):
cdef public object _magma_gens
cdef public dict _magma_cache

cdef _coerce_c_impl(self, x)


cdef class BooleanPolynomialRing_base(MPolynomialRing_base):
pass
84 changes: 50 additions & 34 deletions src/sage/rings/polynomial/multi_polynomial_ring_base.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -571,25 +571,28 @@ cdef class MPolynomialRing_base(CommutativeRing):
else:
return self._generic_coerce_map(self.base_ring())

cdef _coerce_c_impl(self, x):
cpdef _coerce_map_from_(self, other):
"""
Return the canonical coercion of x to this multivariate
polynomial ring, if one is defined, or raise a :exc:`TypeError`.
Return whether there is canonical coercion map
from the ring ``other`` to this multivariate polynomial ring `R`.

The rings that canonically coerce to this polynomial ring are:
The rings that canonically coerce to the polynomial ring `R` are:

- this ring itself
- polynomial rings in the same variables over any base ring that
canonically coerces to the base ring of this ring
- polynomial rings in a subset of the variables over any base ring that
canonically coerces to the base ring of this ring
- any ring that canonically coerces to the base ring of this polynomial
ring.
- the ring `R` itself,

- the base ring of `R`,

- any ring that canonically coerces to the base ring of `R`.

- polynomial rings in an initial subset of the variables of `R`
over any base ring that canonically coerces to the base ring of `R`,

- polynomial rings in one of the variables of `R`
over any base ring that canonically coerces to the base ring of `R`,

TESTS:

This fairly complicated code (from Michel Vandenbergh) ends up
implicitly calling ``_coerce_c_impl``::
Fairly complicated code (from Michel Vandenbergh)::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this is a cpdef method we can probably test it directly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt it's intended to be more of a "try to keep the test" than actually testing the method. But yes, R.coerce_map_from(S) should delegate to this function anyway.


sage: # needs sage.rings.number_field
sage: z = polygen(QQ, 'z')
Expand All @@ -602,27 +605,40 @@ cdef class MPolynomialRing_base(CommutativeRing):
sage: x + 1/u
x + 1/u
"""
try:
P = x.parent()
# polynomial rings in the same variable over the any base
# that coerces in:
if isinstance(P, MPolynomialRing_base):
if P.variable_names() == self.variable_names():
if self.has_coerce_map_from(P.base_ring()):
return self(x)
elif self.base_ring().has_coerce_map_from(P._mpoly_base_ring(self.variable_names())):
return self(x)

elif isinstance(P, polynomial_ring.PolynomialRing_generic):
if P.variable_name() in self.variable_names():
if self.has_coerce_map_from(P.base_ring()):
return self(x)

except AttributeError:
pass

# any ring that coerces to the base ring of this polynomial ring.
return self(self.base_ring().coerce(x))
base_ring = self.base_ring()
if other is base_ring:
# Because this parent class is a Cython class, the method
# UnitalAlgebras.ParentMethods.__init_extra__(), which normally
# registers the coercion map from the base ring, is called only
# when inheriting from this class in Python (cf. Issue #26958).
return self._coerce_map_from_base_ring()

f = self._coerce_map_via([base_ring], other)
if f is not None:
return f

# polynomial rings in an initial subset of variables
# over the any base that coerces in
if isinstance(other, MPolynomialRing_base):
if self is other:
return True
n = other.ngens()
check = (self.ngens() >= n and
self.variable_names()[:n] == other.variable_names())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this assuming that the subset will be the first n?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously the coercion system wasn't entirely self-consistent either. #39126

We can probably fix this first (keep the old non-self-consistent behavior unchanged) then change it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have kept the hypothesis that the subset of variables are the initial ones, and changed the documentation accordingly. This is a progress on what was there before (only exactly the same variables) and still a reasonable assumption to make, with no possible round trips.

if other.base_ring is base_ring and check:
return True
elif base_ring.has_coerce_map_from(other._mpoly_base_ring(self.variable_names())):
return True

# polynomial rings in one of the variables
# over the any base that coerces in
elif isinstance(other, polynomial_ring.PolynomialRing_generic):
if other.variable_name() in self.variable_names():
if self.has_coerce_map_from(other.base_ring()):
return True

# any ring that coerces to the base ring of this polynomial ring
return self.base_ring().has_coerce_map_from(other)

def _extract_polydict(self, x):
"""
Expand Down
Loading