Make Generic.Poly similar and zero steal parents if appropriate. #907
Conversation
Codecov Report
@@ Coverage Diff @@
## master #907 +/- ##
==========================================
+ Coverage 82.34% 82.37% +0.02%
==========================================
Files 86 86
Lines 20045 20047 +2
==========================================
+ Hits 16506 16513 +7
+ Misses 3539 3534 -5
Continue to review full report at Codecov.
|
|
I am not in favor of the different parent comparisons. I still believe removing the zero(p::PolyElem) = zero(parent(p))would have been easier (I proposed this in the other issue). Similar for Edit: Probably this definition I suggest also has some drawbacks. I would like to hear your opinion on that. |
|
I don't see anything wrong with this, nor do I see anything wrong with zero(p::PolyElem) = zero(parent(p))How much pain is there going to be on the nemo side? |
|
@thofma For zero I can understand your perspective, but I don't get what you mean for similar. Similar absolutely has to do what it currently does, and it was actually you that convinced us of that. Consider for example a polynomial over a number field and you ask for a similar polynomial over ZZ. A number field poly is generic but a poly over ZZ is not, so similar can't return the same sort of polynomial. There are also issues in Singular. For example consider a Singular polynomial ring over a Nemo ZZ. You definitely don't want to create an fmpz_mpoly when you call similar there. You want it to use a Singular polynomial type. So similar has to be complicated. I agree zero could work the way you said. I just followed what was done for matrices (i.e. call similar). I'd be happy to change both matrices and polynomials to work the same way, if that's what you want. But existing tests will fail. The other issue that is very important is that the similar system has to be manageable. We spent really a lot of time coming up with a system that does not require n^2 different implementations of similar if there are n polynomial types (e.g. all the Flint and Singular polynomial types). A system which satisfies all these constraints is basically forced to do exactly what similar does right now. The only way we could do better would be to implement polynomials and matrices over number fields in Antic C so that we don't have this issue of them being generic. Then we could have a much simpler similar system. But, well, you know that no one is encouraging such a project, so that's not going to happen. zero could definitely still be changed though, assuming everyone agrees to the change. Similar is not used very much by the user, whereas zero may be, so it is probably worth getting right. |
|
Just to clarify: I have no problem with zero(p) = zero(parent(p)), so long as that is how it works for matrices too! People need to be able to reason about the system, and inconsistency makes that impossible. |
|
As I said, the zero function for matrices arose out of the fact that similar creates a matrix with undefined entries, for performance reasons. This wasn't considered convenient and so zero was born. But as I've pointed out, people can call zero_matrix(base_ring(M), m, n) instead of zero(M, m, n) if they want a zero matrix. It's more typing, but I don't think that matters (apart from the fact that we may have to change some of our existing code to use zero_matrix instead of zero). |
|
And if you are wondering why I can't make zero(M) call zero(parent(M)) and make zero(M, m, n) have the current behaviour, the answer is that I don't know how to implement the latter without calling similar (I actually looked into this, it's really difficult). Therefore zero for matrices would be completely inconsistent and hard to reason about if I did that. |
|
Let's merge this here. We can add similar(x::PolyElem; cached) = zero(parent(x))
zero(x::PolyElem; cached) = similar(x)later if we want to avoid the (small) overhead introduced here. I will probably remove the breaking label, since this is a bugfix in my opinion. |
|
PS.: Before merging I suggest making those |
|
Yeah it's only breaking without the Nemo patches. I'll make the changes after lunch. |
Co-authored-by: thofma <thofma@gmail.com>
|
Thanks. Just out of curiosity, why is this breaking without the Nemo patch? It seems that downstream tests are all passing. |
|
Well I suppose no one is using it, but obviously only Nemo can create Nemo parents correctly. |
Fixes #897
Note that there is a corresponding Nemo fix required for this to pass.
This is the most conservative fix I can come up with which fixes 897 and does what we all agree on.
If this is accepted I will do the same thing for power series.
The system is still inconsistent, but we all agree the current functionality is not useful. The principle here is:
At least this fix is not inconsistent with matrices, given that parents are not stored in the objects anyway. We probably should use @rfourquet's parent comparison idea there instead.
I should point out that I actually don't see a way to have zero and similar return a polynomial with exactly the same parent as the one that is being copied in all cases. Yes, one could steal or deepcopy and modify the parent in other cases too, but then there is no way to know if the stolen parent actually matches the polynomial we are creating, as one has no idea how that original polynomial was constructed.
It's worse for power series, since one might want exactly the same kind of parent, just with a different max_precision or something like that. This problem already occurs for polynomials if you change the variable (not that either of these cases would have been compatible with the original polynomial in the first place).
The only thing you can guarantee is that if the original polynomial was created with an optimal parent for the current package, the new one will have the same parent if it is meant to be compatible with the original.
I believe the current code does at least do that.