Remove _map for universal polys
#2206
Conversation
fingolfin
left a comment
fingolfin
left a comment
There was a problem hiding this comment.
If it passes all tests I am happy to trust your judgement
|
Ok, this is the usual problem with the exponent vector length of the universal polys. I'll look into it. |
|
This additional |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2206 +/- ##
==========================================
- Coverage 88.01% 87.94% -0.07%
==========================================
Files 127 127
Lines 31765 31749 -16
==========================================
- Hits 27957 27923 -34
- Misses 3808 3826 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2206 +/- ##
=======================================
Coverage 88.01% 88.01%
=======================================
Files 127 127
Lines 31765 31746 -19
=======================================
- Hits 27957 27941 -16
+ Misses 3808 3805 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
As suggested by @lgoettgens I've adjusted the |
SoongNoonien
force-pushed
the
remove_map
branch
from
67e8119 to
b193a53
Compare
| return isless(data(a), data(b)) | ||
| end | ||
| S = parent(a) | ||
| num = nvars(S) |
There was a problem hiding this comment.
| num = nvars(S) |
| function change_base_ring(R::Ring, p::UnivPoly{T}; cached::Bool=true, parent::UniversalPolyRing = _change_univ_poly_ring(R, parent(p), cached)) where {T <: RingElement} | ||
| base_ring(parent) != R && error("Base rings do not match.") | ||
| return _map(R, p, parent) | ||
| return UnivPoly(change_base_ring(R, upgrade(AbstractAlgebra.parent(p), data(p)); parent = mpoly_ring(parent)), parent) |
There was a problem hiding this comment.
How about introducing a new helper, like this (untested code)
function upgrade(p::UniversalPolyRingElem)
return upgrade(parent(p), data(p))
endthen calls to upgrade can further be simplified, e.g. here
| return UnivPoly(change_base_ring(R, upgrade(AbstractAlgebra.parent(p), data(p)); parent = mpoly_ring(parent)), parent) | |
| return UnivPoly(change_base_ring(R, upgrade(p); parent = mpoly_ring(parent)), parent) |
Actually I still find this a bit difficult to untangle, splitting this up would help, e.g.
| return UnivPoly(change_base_ring(R, upgrade(AbstractAlgebra.parent(p), data(p)); parent = mpoly_ring(parent)), parent) | |
| p2 = upgrade(p) | |
| p3 = change_base_ring(R, p2; parent = mpoly_ring(parent)) | |
| return UnivPoly(p3, parent) |
and now I realize that there might be a problem, namely if nvars(mpoly_ring(parent)) and nvars(mpoly_ring(AA.parent(p))) differ (this is not a new problem of course, and perhaps best to ignore for now; my point is that the splitting enabled me to reason about the code in a way I couldn't when it was all in one line).
There was a problem hiding this comment.
We can add this new upgrade method but #2198 changes this anyway.
| t = upgrade(S, t) | ||
| end | ||
| return isless(s, t) | ||
| return isless(upgrade(S, data(a)), upgrade(S, data(b))) |
There was a problem hiding this comment.
So we upgrade the underlying polynomials of a and b and then throw this away... I always wondered if that's a good idea. Well, it is certainly wise not to change this in this PR, but perhaps in a future PR we could think about introducing another helper (distinct from the one I suggest in another comment) like this:
function upgrade!(p::UniversalPolyRingElem)
p.p = upgrade(parent(p), data(p))
return p
endand then here we can do
| return isless(upgrade(S, data(a)), upgrade(S, data(b))) | |
| return isless(upgrade!(a), upgrade!(b)) |
There was a problem hiding this comment.
Yes, I thought about this too. I wasn't sure if it is a good idea that isless potentially modifies its inputs. Of course they would only be modified internally but shouldn't isless be named isless! then? (I'm not actually suggesting to rename isless, just wondering.)
There was a problem hiding this comment.
The internal representation may change at any point, as it is internal. We try to not change it during printing as that may lead to hard to track down bugs, but otherwise that is fine to do in a non-! function
There was a problem hiding this comment.
Ok, alright then. I'll adjust the PR accordingly.
There was a problem hiding this comment.
Could you put this, and also the change that upgrade checks if it needs to do anything, into its own PR to not clutter this one here with mostly unrelated changes?
There was a problem hiding this comment.
Yes, I can do this as well.
There was a problem hiding this comment.
In which order would you like to have the changes?
There was a problem hiding this comment.
Maybe first do the other PR about upgrade, we merge that and then update the one here?
3 participants
I noticed that
_mapjust duplicates code which is already present for multivariate polynomials. So, let's just use the present code instead.