-
Notifications
You must be signed in to change notification settings - Fork 61
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
Make Generic.MatSpace truly generic #1318
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1318 +/- ##
==========================================
+ Coverage 87.28% 87.31% +0.03%
==========================================
Files 116 116
Lines 29736 29892 +156
==========================================
+ Hits 25954 26100 +146
- Misses 3782 3792 +10 ☔ View full report in Codecov by Sentry. |
c408c04
to
42a099e
Compare
src/generic/GenericTypes.jl
Outdated
entries = fill(zero(R), r, c) | ||
entries = Matrix{T}(undef, r, c) | ||
for i in 1:r*c | ||
entries[i] = zero(R) |
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.
Note: this is actually a bug fix, so even if we reject this PR here, this part of it should be merged... The bug is that by aliasing some entries, some code won't work right if it uses functions like add!
on matrix entries... I found out because this PR now uses this constructor, so the issue became apparent; in master
it lurks hidden.
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.
Yes, the code in AA assumes that one cannot do add!
on matrix entries per se. If we want to change this, this needs further discussion and maybe do it in a separate PR.
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.
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.
So are you saying that code I found which failed when I aliased the zeros is wrong?
Otherwise, that link says that matrix elements may alias; but it does not prescribe whether e.g. zero_matrix
may produce a matrix with aliased entries; or whether it perhaps must produce one with non-aliased entries?
For now I went with the latter because it fixed a bug, and it also preserves the current behavior, so is less likely to introduce regressions.
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.
Yes, it is probably wrong. But I would have to see it.
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 forgot which one it was, but basically any of the functions in src/Matrix.jl
which invoke e.g. addeq!
likely will not work right when there is aliasing, such as fflu!
, used to implement det
.
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.
if I restore the code with fill
then test/generic/MatrixAlgebra-test.jl:1187
fails:
R, x = polynomial_ring(ZZ, "x")
U, z = polynomial_ring(R, "z")
T = matrix_space(R, 6, 6)
M = T()
for i = 1:3
for j = 1:3
M[i, j] = rand(R, -1:2, -10:10)
M[i + 3, j + 3] = deepcopy(M[i, j])
end
end
p1 = charpoly(U, M)
for i = 1:10
similarity!(M, rand(1:6), R(rand(R, 0:2, -3:3)))
end
p2 = charpoly(U, M)
@test p1 == p2
So maybe only similarity!
is broken?
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.
Hm, maybe the same error as in #955 :) There Claus made things also correct according to the documentation, but it exposed some other bugs.
I will have a look
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 wrote something about this proposed change in #1000
This is now ready to review. |
end | ||
end | ||
for i in 1:min(nrows(a), ncols(a)) | ||
M[i, i] = rb |
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.
of course here we also have aliased entries...
4199f94
to
fd8ff0c
Compare
I've been wondering: right now, this PR is completely geared towards providing a unified But what if there are additional matrix types for the same ring |
I understand how this could be useful, but at the moment I do not see the end. For example, the sparse matrices are covered by https://github.com/thofma/Hecke.jl/blob/master/src/HeckeTypes.jl#L409. At the moment I do not see that we will have plenty of other matrix types, so I would put this on the back burner. |
Yeah, it's not something I'd wish to tackle in this PR... and if we ever want it, it shouldn't be too hard to add. Once the OscarCI woes are resolved (hoping PR #1345 will do that), I'll re-run CI here, and make sure all tests pass, and then I hope we can merge? |
I still am a bit skeptical about those changes with the non-aliasing of the entries. I works around a real bug, by making the code slower and more inconsistent. Inconsistent, because we are adding more code that assumes that entries do not alias each other, while there is also a lot of code which does not assume this. |
fd8ff0c
to
3db3b54
Compare
Re: unaliasing: so just to be clear, this is about restoring the behavior to how it was a few weeks ago: I changed it accidentally as a side effect of some other re-factoring. If you think the accidental introducing of the aliasing I made is a desirable optimization, we can reintroduce it in a dedicated PR, together with similar changes to a bunch of other places where they could obviously be made. But it seems undesirable to have this kind of change as an accident in a commit / PR that does not even mention it (namely PR #1324). In particular this optimization is in 0.29.3 and 0.29.4 and in no other AA version. |
3db3b54
to
eae0a09
Compare
I've removed the |
eae0a09
to
48f19bc
Compare
OK. The downstream error is expected? If so, I will mark this as breaking |
Better hold it for now, I may need to reconsider some things sigh |
0d85312
to
112a3b2
Compare
Hrm, the tests against Nemo release fail because it has silly tests of the form
which now fail because because the "generic" matspace and the regular matspace are the same. I would still argue this does not constitute a true breaking change -- arguably that test is the problem, no real code relies on this. |
I would argue that we should now export |
Nevermind, this needs a breaking release since one needs to merge the abstract and the concrete type for this. |
I think that changing this behaviour would lead to Nemocas/Nemo.jl#651 again. I didn't quite understand what'S going on there, but maybe @thofma remembers. |
I don't think this would lead to Nemocas/Nemo.jl#651 again. That test there was about verifying that adding generic matrices again produces generic matrices. Removing the test won't change that. The problem with that test (and its many variants, all of which are removed by Nemocas/Nemo.jl#1738) is that it tries to instantiate the generic matrix via the generic matrix space, which now is gone. We could change this to try to instantiate the generic matrices directly:
Those generic matrices over e.g. ZZ or QQ of course then wouldn't have a valid parent type anymore, because the change in this PR ensure there is one matrix space per tuple
Therefore I think such test would be somewhat besides the point -- generic matrices over ZZ, QQ, ... should simply not be used anymore. Those tests verified that arithmetic on generic matrices over rings were we have "better" "native" matrix types does not "accidentally" produce matrices of a different type. That is still the case:
Anyway, the test needs to be adjusted, either by removing it (as I do now) or by changing how the initial generic matrix is generated (as done above). What do you prefer, @thofma ? |
Yes, the tests are obsolete or wrong with the changes here (which we want!). |
The failures in HeckeCI seem like something real. Or do we want to abandon the kwarg |
On the long run the But the PR here does not actually remove a As a pragmatic solution, I'll add back the |
... but leave it undocumented on purpose
The HeckeCI changes are resolved. Shall we merge this before making 0.41.4 then? |
For future bisecting/CI reasons, it would be better to first delete/adapt the tests in Nemo, release a new Nemo, then merge this, release a new AA, and then merge the rest in Nemo. But if this is too much overhead and you are sure that Nemocas/Nemo.jl#1738 can be merged immediately after releasing AA and fixes CI and everything, that's fine for me as well. |
I created Nemocas/Nemo.jl#1748 to adapt the Nemo tests. |
Everything is green now. No objections from my side |
Closes #974
Resolves #911