Move generic algorithms out to src. #887
Conversation
wbhart
changed the title
Codecov Report
@@ Coverage Diff @@
## master #887 +/- ##
==========================================
+ Coverage 81.95% 81.98% +0.03%
==========================================
Files 56 86 +30
Lines 19643 19663 +20
==========================================
+ Hits 16098 16121 +23
+ Misses 3545 3542 -3
Continue to review full report at Codecov.
|
|
I haven't followed the whole discussion, but I want to say that this looks good. However, I think I, some months ago, underestimated how many generic algorithms there are (and what does count as generic?), and pushing everything "generic" to Currently I am thinking something along the lines with:
Furthermore, how can achieve clarity in the code and file structure? Currently files like Does this make sense or not? |
|
I will just separate the files. If someone wants to split them at a later date, that is fine by me. I don't particularly have a problem with the long files, but if it is annoying others, than I am ok with them being split. It does make it a little harder to do search and replace if the files are split though. |
|
Agreed, splitting can be done later and should be discussed first. Any idea what to do with the tests @wbhart? |
|
It's quite a lot of work doing this splitting, so I'm not going to consider the tests with this PR. |
|
Sorry, I think the other merged introduced a conflict. We will refrain from merging stuff till you are done here. |
|
It's ok, I was expecting that particular merge conflict. It's not such a big deal because that entire list will go away. |
|
I will try to fix the ambiguity tomorrow. |
|
Thanks. The other possibility is that the code behaves differently now and so you end up in a different code path. But unfortunately I don't understand what the code is intended to do or I'd try to fix it myself. |
|
I've tested locally against Nemo.jl and Singular.jl. I am not able to test against Hecke.jl locally at present. And we already see Oscar.jl fails against this PR on CI. |
|
The problem is that the new polynomial ring functions (or the refactored ones) make PolynomialRing(FlintQQ, 2, ordering = :lex)call the wrong method (it is producing a generic type). One option is to fix it so that it still produces the correct thing. The other option is to mark it breaking and then adjust Nemo. |
|
Yeah, I just found a case in Hecke where it seems R = ResidueRing(ZZ, ZZ(32)) is created but not cached. Then a polynomial ring is created over that which is cached. Then R = ResidueRing(ZZ, ZZ(32)) is created but cached. Then a polynomial ring is created over that. The last one fails because fmpz_mod_poly's cache on modulus not on base_ring. We are going to be digging these issue out for ages. We really have to prevent library code being able to create rings that are cached. |
|
Anyhow, as for the above problem, I will try to fix it. But there are numerous failures in Hecke which are going to be really hard to track down. |
|
I can have a look at those. |
|
Which way do you want to fix it? Mark it breaking and just Nemo or work around it here? |
|
No, it will have to be fixed here. This PR is not meant to be breaking. |
|
Here is the first problem for you to track down (against this PR): This is caused by something like what I said above. |
|
OK, I will have a look. P.S.: Since we have merged #871, the next release will definitely be breaking. |
|
Oh no, I'm wrong. We will have to fix these issues in Nemo. There's no way to fix that here without undoing the improvements I made wrt using symbols instead of strings. |
|
I will prepare a PR for Nemo with the same name. |
|
Your example reduces to (without Hecke): julia> using Nemo
julia> R = ResidueRing(ZZ, fmpz(32))
Integers modulo 32
julia> N = R[1 1; 1 1]; Nemo.AbstractAlgebra.det_df(N)
0
julia> R = ResidueRing(ZZ, fmpz(32), cached = false)
Integers modulo 32
julia> N = R[1 1; 1 1]; Nemo.AbstractAlgebra.det_df(N)
ERROR: Wrong parents
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:33
[2] FmpzModPolyRing
@ ~/.julia/dev/Nemo/src/flint/fmpz_mod_poly.jl:997 [inlined]
[3] PolynomialRing(R::Nemo.FmpzModRing, s::String; cached::Bool)
@ Nemo ~/.julia/dev/Nemo/src/flint/fmpz_mod_poly.jl:1026
[4] PolynomialRing
@ ~/.julia/dev/Nemo/src/flint/fmpz_mod_poly.jl:1024 [inlined]
[5] det_df(M::fmpz_mod_mat)
@ AbstractAlgebra ~/.julia/dev/AbstractAlgebra/src/Matrix.jl:1756
[6] top-level scope
@ REPL[6]:1Edit: Easier julia> R = ResidueRing(ZZ, fmpz(32))
Integers modulo 32
julia> PolynomialRing(R, "x")
(Univariate Polynomial Ring in x over Integers modulo 32, x)
julia> R = ResidueRing(ZZ, fmpz(32), cached = false)
Integers modulo 32
julia> PolynomialRing(R, "x")
ERROR: Wrong parents
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:33
[2] FmpzModPolyRing
@ ~/.julia/dev/Nemo/src/flint/fmpz_mod_poly.jl:997 [inlined]
[3] PolynomialRing(R::Nemo.FmpzModRing, s::String; cached::Bool)
@ Nemo ~/.julia/dev/Nemo/src/flint/fmpz_mod_poly.jl:1026
[4] PolynomialRing(R::Nemo.FmpzModRing, s::String)
@ Nemo ~/.julia/dev/Nemo/src/flint/fmpz_mod_poly.jl:1024
[5] top-level scope
@ REPL[5]:1 |
|
I will make a PR to fix it in Nemo, OK? |
|
Yes, go ahead. Thanks for tracking it down. I have a PR to make Nemo accept strings and symbols everywhere. Just testing it now. |
|
Hmm, same bug. What have I done wrong I wonder. |
|
I would have guessed that the previous constructors were correct. |
|
Calling out to Generic? I'm not so sure. I think we want to always call the versions with symbols and with the new PR for Nemo that will be the case now. |
|
This is looking good now! Will merge if there are no objections. |
|
Yes please. |
Fixes #785
So far the following files have been split:
Notes:
This is a massive project. Splitting the files is not the hard part. Dealing with all the imports/exports, tracking down bugs that arise, carefully removing unnecessary AbstractAlgebra and Generic disambiguations, dealing with documentation, etc., are the hard parts. To keep the project manageable I've focused on doing the bulk of the project without getting too bogged down in minutiae. Otherwise the project will grind to a halt and hold up others implementing stuff for AbstractAlgebra.
This is a first order approximation. For example, we might later decide that functions such as similar, zero, polynomial, which generate generic types belong in the generic module. The reason I haven't put them in there now is that these are the generic functions which other packages will need to overload (e.g. Nemo), and it is natural that they belong in AbstractAlgebra. They are generic fallbacks, if you like, in case those packages don't implement something specific. To reflect this, the functions themselves could possibly be split into two parts, one in src/generic the other in src. As it is, even though they can only generate generic types, the functions still provide functionality to all types implemented by other packages. So for now I've judged them to belong to AbstractAlgebra not Generic.
There are a handful of AbstractAlgebra.blahs still left in Generic files. These could be omitted in some cases by importing the relevant symbols into Generic. Someone else can look into this if they care to.
I have introduced CommonTypes.jl. This is a little unfortunate. It contains the definition of Perm and CycleDec which are types needed by matrices. The problem is that functions for matrices cannot refer to Generic types in their signatures (it's fine to refer to them in their bodies), because types must be defined before use. But Generic types don't get defined until after all the files in AbstractAlgebra, as Generic needs to import those functions to overload them. It may be possible to define things in the other order so that this problem goes away, but I could not get it to work easily as things currently are. This is something that could be looked at by someone in a later PR.
I have not attempted to split the test code. Probably cleaning up the test code should be combined with checking that our generic types actually conform to the interfaces they are supposed to, and that unsafe operators are properly tested (especially alias tested), which in many cases they are not. Again, this is for someone in a later PR.
I have not dealt with the files that have arisen from previous partial attempts at this project.
There are some functions that I've moved in Generic purely because there is no abstract type (e.g. AbsSeriesRing) that could be used in their signature. There is nothing in their implementation which prevents them being fallbacks for all AbsSeries, etc. This is something that could be fixed.
In src/RelSeries.jl, set_valuation!, set_precision!, set_length! function as generic fallback functions for all series types, assuming they have val, prec and length fields. I don't know if this is desirable or not. At the end of the day we can make arbitrary assumptions about the types people might implement following an interface. But providing ones like this might just cause more grief when they discover there are functions not in the required interface that they actually need due to the contents of the struct they have implemented. I prefer to give some good examples of creating rings that follow our interfaces, rather than oversimplifying the required interfaces, which basically defeats the purpose of having them.
Two of the Submodule constructors (sub) had to be left in AbstractAlgebra.jl because they refer to the Generic.Submodule type in their signatures and so must be defined after Generic. Similarly for one of the snf functions in InvariantFactorDecomposition.jl.
The distinction between Generic and Abstract in the case of (finitely presented) Modules and ModuleHomomorphisms is not very clear. There is no clear interface for these things. On the one hand, for finitely presented modules, for example, there are numerous concrete types belonging to the abstract type FPModule. But the implementation depends heavily on a particular representation of the data rather than an interface, which means the abstract code is not particularly general here. There are also hacks like the _matrix function (implemented as a type stable accessor) which are called from supposedly abstract code. The map field of the module types is also accessed directly instead of via an interface. This is surely not intended to be part of an official interface. The nature of finitely presented modules is such that they are sufficiently diverse that one probably doesn't want a terribly strict interface, making them essentially unsuitable for truly generic code. One wants at best some general uniformity, rather than a strict interface.
Maps are designed to be completely general, but a lot of the Generic code provides functionality for all maps (e.g. generic compositions), which can be somewhat confusing. I've taken the approach that code which implements functionality (no matter how generic its intended use) for concrete Map types is Generic, not Abstract. Only code which is representation independent which is written to a Map interface is considered truly abstract.
I took the liberty of making all constructors like PolynomialRing which accept strings to also accept chars and symbols, with the Generic version being the symbol one, for performance reasons. Fixes Symbols instead of Strings when constructing polynomial rings #48
In src/generic/MPoly.jl, some of the evaluation code would work just fine in src/MPoly.jl, but < Julia-1.5 doesn't support adding methods to abstract types and therefore this currently can't be supported. When we finally make Julia-1.5 the minimum requirement, we can move this code over.