Small fixes #77
Small fixes #77
Conversation
so that after `using Oscar` one can use Polymake.pm_Matrix... etc directly
|
I assume the export Polymake is just temporary. Obviously we don't want this to be the eventual way that we access Polymake from Oscar. But it's fine for now, of course. Does @micjoswig also need some other packages exported for his demo? Do you want to uncomment the import Singular also for this PR? I don't understand the test failure. I just installed Nemo 0.16.2 on my machine and it gives an ArgumentError, not an AssertionError. So something is going wrong, but I'm not sure what. I don't think Hecke is overwriting something, but I'll check. |
|
Thanks very much, Bill, for asking. No, I don't need anything.
Actually, on purpose, I will not work with Oscar itself, but rather with the various corner stones directly. In this way I want to point out some aspects that I find important for Oscar's design. I will also try to hint at choices that we may have and such.
The beauty of it: it just works, by avoiding a few pitfalls such as
oscar-system/Singular.jl#187
Best,
Michael
…On 21.01.20 13:18, wbhart wrote:
I assume the export Polymake is just temporary. Obviously we don't want this to be the eventual way that we access Polymake from Oscar. But it's fine for now, of course. Does @micjoswig also need some other packages exported for his demo?
Do you want to uncomment the import Singular also for this PR?
I don't understand the test failure. I just installed Nemo 0.16.2 on my machine and it gives an ArgumentError, not an AssertionError. So something is going wrong, but I'm not sure what. I don't think Hecke is overwriting something, but I'll check.
--
Prof. Dr. Michael Joswig <joswig@math.tu-berlin.de>
Technische Universitaet Institut fuer Mathematik, MA 6-2
Str. des 17. Juni 136 D-10623 Berlin, Germany
phone +49 (30) 314-75904 fax +49 (30) 314-25047
|
|
The test failure is due to factor being imported from Hecke now instead of Nemo. This caused the tests to fail. This is the offending commit: 4b92151#diff-e2d778cdd99534bb69ad0935780c53f2 It's a bit annoying that Julia didn't give me a warning about the conflicting definitions of factor. |
|
I don't understand why everything was imported from Hecke, but the same was not done for Nemo, Polymake and other packages in Oscar. |
|
Julia is not giving a warning, since nothing is overwritten. The released Hecke versions are not overwritting any functions/methods (just like AbstractAlgebra is not overwritting functions from Base). |
|
I really can't follow the architecture who imports what from whom, so it's your job to decide where the failure come from and what is the best fix. Also: You tell me if Singular should be imported or not. This probably doesn't belong here, but: I'm not sure how close |
|
@kalmarek Yeah, it's not your job to figure out the Hecke issue. My comments there are for @fieker. Sorry for not addressing them to him directly. Regarding import Singular, yeah someone else told me that Singular.jl works on Julia 1.3 for them, which surprised me a lot. Either way it doesn't matter as we don't have 1.3 in our test matrix. So you can uncomment that. And you can also import Gap. [The only reason Gap isn't already there is it takes 20 minutes (including the Gap packages) to build which is way too slow. And I finally recalled why I removed Singular.jl. It also takes too long to build (even with BinaryBuilder) and I was trying to minimise the turnaround time on building the Oscar docs so that I didn't have to wait half an hour for every single change I made to the documentation before I could see it online. But I'm not working on that at present, so it can go back to importing Singular.] So to summarise: could you please add import Singular and import Gap to Oscar.jl in your PR. We will have to wait for a new Hecke release to fix the failing tests. @thofma is possibly working on this. For large factorisations, the Hecke code is many times slower than Nemo. For smooth factorisations it is many times faster. Hecke needs the latter. [I continue to work on the Flint release, which will once and for all fix the issue so that all factorisations are fast.] Let me just check Gap.jl is in the dependencies of the project. I don't recall if I ever got around to adding it. Other than that, we will be right to go, I think. And thanks for your help. I've literally gotten nothing done today, and that includes what I am supposed to be working on, which is Flint. Your help is invaluable. |
|
Hmm, no Gap is not in the dependencies. So I think we need to add the following in [deps]: Gap = "c863536a-3901-11e9-33e7-d5cd0df7b904" and in [compat] add: Gap = "^0.2.3" Could you possibly also add these in Project.toml in your PR @kalmarek The Hecke release should in theory be independent, but I guess we may as well wait until it is done so we can check the Oscar tests actually pass before doing the Oscar release. |
|
ok, I'll do; regarding the time spent compiling GAP: after oscar-system/GAP.jl#313 is merged the time for build should not exceed ~2 minutes (which is ok for the user, but maybe still not for developing). As for the factorisation: if there are some heuristics guiding the choice of algorithm it's best resolved by |
|
@kalmarek Thanks for pointing out oscar-system/GAP.jl#313. That looks positive. As for factoring, the strategy cannot be handled at a high level. Factoring calls back into itself from deep within the C code, and I cannot call a high level Julia function from within a Flint C function. The problem has to be fixed on the C level (unless you want to rewrite the 20000-30000 odd lines of code in Flint for this in Julia. Having said that, I've made some temporary changes to the Hecke factoring routine with the help of @thofma so that it isn't so bad for large inputs and yet shouldn't be much worse for smooth factoring. In this state it could be moved into Nemo. But ultimately the only non-bandaid solution is for me to finish the Flint release. |
|
I probably should have mentioned: given an input integer n, there is no heuristic for choosing which algorithm to call. Unless you know the factorisation of n, you have no idea what algorithm to call. So that's just not how factoring algorithms work. Instead, they work by running factoring algorithms with small parameters to gather probabilistic information about n and once any factor is found, remove it and reenter the factoring routine at a point that takes into account all the previously determined information. Integer factoring is one of those big "engines" that can consume the efforts of many developers. By the way, the latest flint code can parallelise the factoring. For example, on a 4 core laptop you can get the factors of 90 digit numbers (for the worst kind of input) in just over 20 minutes. This isn't in Nemo yet, as the Flint binaries haven't been updated. But it will be in soon. |
|
yep, I didn't mean the specific |
|
Sorry, I am being dumb. I don't understand what you mean by A\b. |
|
this function https://github.com/JuliaLang/julia/blob/2d5741174ce3e6a394010d2e470e4269ca54607f/stdlib/LinearAlgebra/src/generic.jl#L1036 |
|
By traits, you meant runtime properties of the values, not type traits, I guess. But I don't see what this has to do with factor. There are no different algorithms based on properties of the numbers, at least not ones you can guess without first factoring the numbers. This wasn't about two different implementations of factor, one for numbers which were even and one for numbers which are odd, say, which you can choose between at runtime based on whether the number is odd or even. This was about one implementation of factor which mainly cares about small factors of a number and another which cares only about large factors. One implementation was replaced with the other in Oscar. This should not have happened, especially since it was based on a known buggy implementation of ECM in Flint, and the implementation of factor was buggy and didn't really make any sense. (I'm being reasonable here, aren't I?) Thanks to @thofma for helping me improve it. [I have removed other comments above about this issue because one of the Hecke developers pointed out to me privately that my comments reflect unfairly on their own efforts, which were unrelated to the problems I was discussing. ] |
|
I can't answer your questions, but this pull is about small fixes and getting Oscar a new version; please review and merge if appropriate. |
|
The tests are failing since we test julia 1.0 and GAP.jl version v0.2.3 requires julia 1.1. |
|
Oh that's a big shame. I thought they backported all the fixes we needed to 1.0. We really need to ask them to do this. Not supporting 1.0 is a major problem as it is the LTS version of Julia. |
|
Having thought more about this issue, I strongly believe that if we import everything from one of our packages, we should import everything from all our packages into Oscar. Either that or we should only import the stuff we actually want in Oscar, as needed, from the individual packages. I strongly prefer the second option, but I'll live with the first if absolutely necessary. But I believe this should be rectified before we do the release. It will demonstrate our commitment to an equal treatment of our respective cornerstones and will certainly be in the spirit of collaboration. |
|
ok, guys I believe You can take it from here up to the "0.1.1", good luck! |
|
@kalmarek Thanks for your help. We'll merge this and issue the release as soon as we can. We'll deal with my final comment at some later time (I still believe it is important) after discussing it at a development meeting. For now, it is better we don't hold the release up for that. By the way, did you increment the Julia minimum requirement to 1.1 and the test matrix in travis.yml to Julia 1.1? I think that should hopefully make the tests pass. |
|
@kalmarek Thanks. We now seem to have "ERROR: Compat |
|
The other issue looks equally bizarre. A GC crash, which I've not seen before. Probably due to the GC integration for GAP.jl? |
|
indeed this looks bizarre, (reference to Polymake in the middle of GAP build?!) anyway: I restarted those jobs, let's see what tomorrow brings |
|
@wbhart I've added you as collaborator to my fork, please commit at will |
|
Thanks |
|
Restarting the build hid the GC issue, which I think @rbehrends and @fingolfin should have known about. Who is responsible for this at the moment? The Polymake issue seems easy to fix, i.e. just do what it says. @fieker @thofma @rfourquet @fingolfin Probably I need to be more explicit that I am not working on Oscar until the end of March. (I realise that is not what I said above, but things subsequently changed due to things that were discussed offline.) |
|
I'll have to look at it and see if I can reproduce it; the backtrace doesn't provide enough information to pinpoint the root cause. While it reports the issue as a GC error, the underlying problem is a wrong type header word in an allocated object that can be caused by a number of things, including simple off by one writes in regular C code. Basically, each allocation in Julia is preceded by a header word that describes the Julia type of the object. As pool objects are packed and contiguous, both writing past the end of the object or just in front of the beginning can alter such a header word and cause the GC to panic; the GC can't proceed, as it needs to know the object type for marking it. |
configure: installing GAPTypes.jl
│ ERROR: Compat `Polymake` not listed in `deps` or `extras` section.
│ Stacktrace:
│ [1] pkgerror(::String) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/Types.jl:112
│ [2] validate(::Pkg.Types.Project) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/project.jl:107
│ [3] Pkg.Types.Project(::Dict{String,Any}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/project.jl:123
│ [4] #read_project#37(::String, ::typeof(Pkg.Types.read_project), ::IOStream) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/project.jl:140
│ [5] #read_project at ./none:0 [inlined]
│ [6] #40 at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/project.jl:144 [inlined]
│ [7] #open#312(::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}, ::typeof(open), ::getfield(Pkg.Types, Symbol("##40#41")){String}, ::String) at ./iostream.jl:375
│ [8] open at ./iostream.jl:373 [inlined]
│ [9] read_project(::String) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/project.jl:143
│ [10] Pkg.Types.EnvCache(::Nothing) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/Types.jl:318
│ [11] Type at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/Types.jl:314 [inlined]
│ [12] Pkg.Types.Context() at ./util.jl:723
│ [13] #add#24 at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/API.jl:67 [inlined]
│ [14] add at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/API.jl:67 [inlined]
│ [15] #add#21 at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/API.jl:65 [inlined]
│ [16] add at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/API.jl:65 [inlined]
│ [17] #add#20(::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}, ::typeof(Pkg.API.add), ::String) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/API.jl:64
│ [18] add(::String) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/Pkg/src/API.jl:64
│ [19] top-level scope at none:1
│ configure: error: failed to install GAPTypes.jl
│ ERROR: LoadError: failed process: Process(`./configure --with-gc=julia --with-julia=/home/travis/julia/bin`, ProcessExited(1)) [1]
│ this is GAP.jl messing with global environment while being called in the local one, I don't think it should (or even could) be fixed in Polymake. @fingolfin could you have a look at this? |
|
Sorry, I never noticed that I was @-mentioned here. Anyway, GAPTypes.jl is retired, and GAP doesn't mess with any environments anymore, so at least the last mentioned problem should be gone for good. |
| - 1.0 | ||
| # - 1.2 | ||
| - 1.1 | ||
| - 1.2 | ||
| # - 1.3 |
There was a problem hiding this comment.
I guess we'd want this to list 1.3, 1.4 and nightly now
| AbstractAlgebra = "^0.8.0" | ||
| Hecke = "^0.7.0" | ||
| GAP = "^0.2.3" |
There was a problem hiding this comment.
| GAP = "^0.2.3" | |
| GAP = "^0.3.5" |
|
I think this is obsolete now. (OK, we haven't added GAP as dependency yet but that'll come once the package registration went through) |
No description provided.