Fix sub and quo for modules #3237
Conversation
|
@HechtiDerLachs Thanks for doing this. I have a general problem with the discussions and the work going on right now. Singular is successful because it is effective. Now, in OSCAR, we make general changes for consistency, but may be loosing effectivity or winning effectivity , depending on what we do. These changes are forced upon a group such as that in commutative algebra whose members, at least currently, do not all have the time to overlook all consequences. I am grateful for all what is being done, but if we continue with this developing scheme, we have no control of the consequences because we do not have test examples which are at the border of what can be done, and such examples are in particular not known to the non-specialist. Imagine me overwriting code in group theory, for example. And imagine a system, which is perfect from a computer science point of view, but cannot work out any interesting research example. |
|
@wdecker : I agree with what you say, but I do not see how this is relevant in this particular PR. This is just to make the outer appearance consistent. It changes nothing about the actual functionality or performance. Edit: It was maybe a bit stupid to also put the fixes in here which can now be found in #3238 . This was simply due to me being on this branch when the questions came in via mail and I couldn't easily change the branch since I was waiting for tests to finish. This is now resolved by putting all those changes in the separate PR #3238. They should only change performance for the better and the bottlenecks I found were mainly due to some programmers lack to care for effectiveness. The related issues have been raised a long time ago and nobody has looked into them (#2999). So the problems I see are not people meeting up and discussing the streamlining of functions throughout Oscar with eventually unforeseen bad consequences in performance. The problem, at least in this particular case, is that nobody picks up on the actual performance issues which are already on the table. This was similar in #3041: The performance was bad, because nobody looked closer into the inner design of the modules and their maps for a long time. But I could find no instance where the Oscar design requirements in terms of user facing functionality had a negative effect forced upon the performance of the code. Again a similar thing in #2879: We discovered that there is a bottleneck in the translation of Koszul complexes from singular to Oscar due to the use of dense matrices. Still not addressed and performance related. But no contradiction to the user-facing design decisions of Oscar. I could go on with this list (see #3211) and we will probably find similar things again when resolving e.g. #2999. So I agree that people should probably also focus on performance issues and analysis and not spend all their time and energy on discussions about what things should look like. But I disagree with that decisions on the outer appearance of functions like |
I guess you are talking about the R, (x, y) = QQ[:x, :y]
F = FreeMod(R, 1)
function fmorph1(F::FreeMod)
I, inc = sub(F, [x*F[1]])
@test haskey(F.incoming, I)
@test Oscar._recreate_morphism(I, F, F.incoming[I]) == inc
@test haskey(I.outgoing, F)
@test Oscar._recreate_morphism(I, F, I.outgoing[F]) == inc
I = 5
inc = "a"
end
fmorph1(F)
GC.gc()
GC.gc()
@test length(keys(F.incoming)) == 0The reason is that julia might not consider the objects as "garbage" at the point where Not really sure about the second test, I guess there is more to it regarding that inclusion? function fmorph2(F::FreeMod)
I, inc_I = sub(F, [x*F[1]])
J, inc_J = sub(I, [x^2*I[1]])
@test haskey(J.outgoing, I)
@test haskey(I.incoming, J)
@test Oscar._recreate_morphism(J, I, J.outgoing[I]) == inc_J
@test Oscar._recreate_morphism(J, I, I.incoming[J]) == inc_J
I = 5
inc_I = 6
return J, inc_J
end
J, inc_J = fmorph2(F)but that doesn't seem to work. |
|
Is there even a guarantee that things will be gc'ed? These tests look a bit brittle. |
|
I think we can safely assume that objects will be considered dead once the scope ends, and once they are considered dead they will be freed with the next garbage collection. The julia testsuite also uses this pattern in several places, with a small function using an object and then checking that it is properly freed after an explicit |
Yes, the second one can come later. Thanks for having a look! |
|
I partly restored the From my side this is good to be merged, but @jankoboehm and I will probably have another look into this PR tomorrow, so we postpone merging at least for today. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3237 +/- ##
========================================
Coverage 81.62% 81.63%
========================================
Files 546 546
Lines 73437 73546 +109
========================================
+ Hits 59946 60039 +93
- Misses 13491 13507 +16
|
…hs/Oscar.jl into fix_sub_and_quo_for_modules
|
doctests still fail |
|
FYI you can run the doctests locally, too, combined with |
|
@fingolfin : Thanks for the hint. But I can run almost no tests locally anymore. My machine has become too small. Plus I am developing two branches simultaneously and switching branches makes a restart of OSCAR mandatory. I get up to 400+ seconds of precompilation time plus Revise still does not work for me in many cases due to unforeseeable hickups. I prefer to have the tests run elsewhere at the moment. |
|
@jankoboehm : Please merge. |
|
We should keep truncate with both options for the moment. I guess this was just changed by accident. |
Looking at it again, the reasons to remove the But it is, what it is. I hope nobody else finds further such nitpicks which prevent this from being merged. |
So do I frequently, or more. My hint: you can have multiple Oscar worktrees in different directories. This way you don't have to switch back and forward. Look for the If you need a faster work machine (desktop or laptop), of course we can get you one. We can work that out the next time you are in KL. |
|
Merged as part of #3298 . |
As requested in #3118 I streamlined the uses of
subin the modules to not accept thetaskkeyword anymore.Unfortunately it turned out that
taskwas also (ab-)used to indicate whether or not canonical maps would be cached. To resolve this, I decided that all maps would now be cached either way. But this will lead to memory cluttering, eventually, so one has to take care to design things so that the garbage collector can pick stuff up when applicable.To allow for
gcto go into action, I switched from a list of maps being stored in the modules toWeakKeyIdDicts with the (co-)domains as keys. Unfortunately, this forbids us to store the actual maps as values as they contain references to both their domain and codomain. So instead I store aTupleconsisting of a sparse matrix and an eventual ring map from which one can restore the morphism using the internal method_recreate_morphism.Tests in the REPL yield that garbage collection works correctly now. But for some reason the non-interactive tests still fail. @benlorenz : Do you maybe have an idea why?
Next step will be cleaning up the
quo. But that could also come in a later PR.Edit: I introduced a function called
submoduleto create just the submodule without the map. It is referred to in the docstrings forsub.