-
-
Notifications
You must be signed in to change notification settings - Fork 532
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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):: | ||
|
||
sage: # needs sage.rings.number_field | ||
sage: z = polygen(QQ, 'z') | ||
|
@@ -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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this assuming that the subset will be the first There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
""" | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.