trying to get rid of coerce_c_impl #39419
Conversation
|
Documentation preview for this PR (built with commit 293b259; changes) is ready! 🎉 |
|
Sounds like a good idea (I keep wondering how does the coercion work for that parent, and why changing Looks like all tests pass anyway. |
|
Ok, so tests pass. I have some concerns about possible speed regression. Note that this is a little step in the removal of the "auld coercion system" that was mostly but not completely replaced by a better one using categories a long time ago. |
| - 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. | ||
|
|
||
| TESTS: | ||
|
|
||
| This fairly complicated code (from Michel Vandenbergh) ends up | ||
| implicitly calling ``_coerce_c_impl``:: | ||
| Fairly complicated code (from Michel Vandenbergh):: |
There was a problem hiding this comment.
Now that this is a cpdef method we can probably test it directly?
There was a problem hiding this comment.
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.
| return True | ||
| n = other.ngens() | ||
| check = (self.ngens() >= n and | ||
| self.variable_names()[:n] == other.variable_names()) |
There was a problem hiding this comment.
Isn't this assuming that the subset will be the first n?
There was a problem hiding this comment.
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.
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.
|
can we please move forward here ? |
|
sure, thanks |
sagemathgh-39419: trying to get rid of coerce_c_impl not seriously tested so far ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. URL: sagemath#39419 Reported by: Frédéric Chapoton Reviewer(s): Frédéric Chapoton, Michael Orlitzky, user202729
not seriously tested so far
📝 Checklist