Add fit! for all MPolyRingElem subtypes
#1823
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1823 +/- ##
==========================================
+ Coverage 86.08% 87.08% +0.99%
==========================================
Files 98 99 +1
Lines 36398 44517 +8119
==========================================
+ Hits 31334 38768 +7434
- Misses 5064 5749 +685 ☔ View full report in Codecov by Sentry. |
| @@ -145,6 +145,11 @@ for (etype, rtype, ftype, ctype) in ( | |||
| a, parent(a))) | |||
| end | |||
|
|
|||
| function fit!(a::($etype), n::Int) | |||
| # needs to exist for the MPoly interface | |||
| return nothing | |||
There was a problem hiding this comment.
If an empty method is a valid default implementation, then why don't we just add fit!(::MPolyRing) = nothing to AbstractAlgebra instead of this PR here?
There was a problem hiding this comment.
from the way that the MPoly interface is formulated in the AA docs, it seemed to me that this function should not have a default implementation. Here, it is fine to do it like this, since Flint does not have the need of this getting called.
The MPoly interface (https://nemocas.github.io/AbstractAlgebra.jl/dev/mpoly_interface/#Unsafe-functions) states that these must exist. When trying to run the UnivPoly testsuite from AA with Nemo rings to verify the correctness of my changes for Nemocas/AbstractAlgebra.jl#1746, there were a lot of MethodErrors due to these missing.
One could potentially look in FLINT if there are corresponding functions there that can be hooked up, but that is not needed currently.