Prep flint 2.7: resolves #860, resolves #870 #919

tthsqe12 · 2020-11-28T14:50:18Z

resolves #860, resolves #870
I'll do the nmod_mpoly/gfp_mpoly thing in a later pr.
One thing at a time

thofma · 2020-11-28T14:56:06Z

Great, thanks.

rfourquet · 2020-12-01T13:00:15Z

src/flint/fq.jl

+    square_root(x::fq)
+> Return the square root of $x$ in the finite field. If $x$ is not a square
+> an exception is raised.
+"""


Suggested change

square_root(x::fq)

> Return the square root of $x$ in the finite field. If $x$ is not a square

> an exception is raised.

"""

square_root(x::fq)

Return the square root of $x$ in the finite field. If $x$ is not a square

an exception is raised.

"""

rfourquet · 2020-12-01T13:01:08Z

src/flint/fq.jl

+    issquare(x::fq)
+> Returns `true` if $x$ is a square in the finite field (includes zero).
+"""


Suggested change

issquare(x::fq)

> Returns `true` if $x$ is a square in the finite field (includes zero).

"""

issquare(x::fq)

Return `true` if $x$ is a square in the finite field (includes zero).

"""

tthsqe12 · 2020-12-02T11:33:13Z

src/flint/fmpq_mpoly.jl

+   dic = Dict{fmpq_mpoly, Int}()
+   for i in 0:fac.num-1
+      f = R()
+      # use get_base instead of swap_base if convert is to preserve input


@rfourquet Is it ok if this function destroys its input? I just didn't want make another useless copy of the polynomials...

Why did you implement a convert method? Or was it there before?

if I implement a convert function, there is less copy+paste and fewer lines of code

The problem with convert is that it will be called implicitely without the users knowledge (for example in array or dictionary assignments), making the destructive behavior rather unfortunate.

How about implementing (::Factor{fmpq_poly})(::fmpz_poly_factor)? Or convert_and_destroy, something that indicates that the input will be garbage.

Yes I agree with @thofma

src/flint/fq.jl

thofma · 2020-12-02T17:55:31Z

Sorry, I was a bit sloppy with my comment. The signature I meant was

(::Type{Fac{nmod_mpoly}})(f::nmod_poly_factor)

So that one calls it as Fac{nmod_poly}(f).

I can push those changes if you want.

Project.toml

@@ -24,7 +24,7 @@ AbstractAlgebra = "^0.11.1"
 Antic_jll = "~0.2.2"
 Arb_jll = "~2.18.1"
 BinaryProvider = "0.4, 0.5"
-FLINT_jll = "~2.6.3"
+FLINT_jll = "~200.690.0"
 LoadFlint = "^0.3.4"


thofma · 2020-12-11T13:47:50Z

Sure, there are a few other changes that are necessary.

Project.toml

@@ -24,7 +24,7 @@ AbstractAlgebra = "^0.11.1"
 Antic_jll = "~0.2.2"
 Arb_jll = "~2.18.1"
 BinaryProvider = "0.4, 0.5"
-FLINT_jll = "~2.6.3"
+FLINT_jll = "~200.690.0"
 LoadFlint = "^0.3.4"


thofma · 2020-12-11T18:11:01Z

Good to go from my side.

thofma · 2020-12-11T18:15:27Z

The failure on nightly looks scary. Might be due to some bad choices in the random test. I will try to reproduce it locally.

fingolfin · 2020-12-11T18:38:59Z

The problem is that fq_ctx_init_modulus expects 4 arguments but gets only 3.

thofma · 2020-12-11T18:41:17Z

Thanks. I will fix it. Weird that it passed for the other jobs.

fingolfin · 2020-12-11T18:50:54Z

Actually it makes me concerned, I now wonder what other similar issues are hidden there...

thofma · 2020-12-11T18:52:12Z

@tthsqe12 please check my fix.

fingolfin · 2020-12-11T19:12:42Z

I've submitted PR #923 to enable code coverage tracking. That shows (see https://codecov.io/gh/Nemocas/Nemo.jl/tree/9e9e763d9cb56919ebf94ce81f985c3a6e4a405d/src) that about 80% of the code base is already covered by tests, which is good. But on the downside that means 20% are not yet tested at all.

Of course just because a line of code is executed as part of a test doesn't ensure it is bugfree (as the issue at hand demonstrates), but at least it's slightly less likely than when the code is never tested...

I really wonder if there is a technology that might help with catching these API mismatches: wrong number of arguments, wrong types... I guess it would have to be code that parses C header files to compute the corresponding data. And then somehow match it to our ccalls. Hrm

fingolfin

Thanks @tthsqe12 @thofma, this seems to work now

tthsqe12 · 2020-12-11T23:37:07Z

@thofma looks fine. I am sorry I missed that one. It would be nice to have some of these errors caught automatically...
If fq_ctx_init_modulus were being tested by the test code, there is no way it would have passed with my buggy omission. What is going on with the test code

tthsqe12 · 2020-12-12T01:40:33Z

@fingolfin I know for a fact that there are other mismatched signatures even from before version 2.6. But, they work currently :) Here are the unpleasant secrets of nemo in order:

We have manually make sure all ccalls are correct
We have to pack (unpack) all of the nested c structs by hand and pray that julia uses the exact same offsets as whatever compiler compiled the c library
Nemo is sneakily relying on ostensibly undefined behaviour No context for destructors of major types flintlib/flint#773

I will go through all of the ccalls soon and remove the nonexistant functions and fix the bad arguments. I didn't want to change too much right now.

fingolfin · 2020-12-15T10:29:50Z

There are still a bunch of code change suggestions by @rfourquet which someone with the right permissions could apply

Co-authored-by: Rafael Fourquet <fourquet.rafael@gmail.com>

thofma · 2020-12-15T10:40:00Z

I don't see square roots for finite fields. Did you add them @tthsqe12?

tthsqe12 · 2020-12-15T10:57:23Z

This only thing missing is issquare_with_square_root in fq.jl and fq_nmod.jl.
By the way, where are the code coverage reports now?

thofma · 2020-12-15T12:19:03Z

Would you mind adding it? That would be great.

fingolfin · 2020-12-15T12:25:34Z

@tthsqe12 the Codecov comment above is updated whenever this PR is rebuilt. It also contains links to the full report: https://codecov.io/gh/Nemocas/Nemo.jl/pull/919 for this PR.

On https://codecov.io/gh/Nemocas/Nemo.jl/pull/919/diff you can just see the changes introduced by this PR, and whether they are covered or not. They currently have a coverage of 90.75% which is above average! Here's an example of a ccall which is not exercise by the test suite: https://codecov.io/gh/Nemocas/Nemo.jl/pull/919/diff#D2-841

Let me know if you have any questions about Codecov, I am quite familiar with it (including a lot of annoying warts... but hey, it's free)

fingolfin · 2020-12-15T12:26:46Z

(Also, while I of course think it'd be useful to increase coverage, getting to 99% is quite ambitious, and perhaps not the best idea to hold up this PR too much just to increase coverage? We can increase it further later, and add corresponding fixes then as well.

@fingolfin

@fingolfin `200.690` is too old to push Nemocas/Nemo.jl#919 over the finish line. Did I bump the version correctly? Or should I bump the patch version?

thofma · 2020-12-16T15:13:30Z

This is now waiting for a new flint version, see JuliaPackaging/Yggdrasil#2265.

wbhart · 2020-12-16T15:56:53Z

The Flint release is waiting on a final PR from @tthsqe12

I think that should be the final issue to deal with and then we can issue a new rc and final release.

@fingolfin

* [FLINT] new version @fingolfin `200.690` is too old to push Nemocas/Nemo.jl#919 over the finish line. Did I bump the version correctly? Or should I bump the patch version? * Update build_tarballs.jl * Update build_tarballs.jl * Update build_tarballs.jl * Update build_tarballs.jl

fingolfin · 2020-12-21T11:23:24Z

New Flint and LoadFlint are out. Anything else that needs to be done?

thofma · 2020-12-21T11:38:23Z

I pushed the last commit this morning to make everything green. I will do some cleaning up later this day.

tthsqe12 · 2020-12-21T13:14:07Z

Yay!

tthsqe12 added 2 commits November 28, 2020 12:16

update interfaces to 2.7

ce45f0a

add sqrt and factor sqrt in 2.7

5a51e3c

rfourquet reviewed Dec 1, 2020

View reviewed changes

tthsqe12 added 2 commits December 1, 2020 15:06

fix some docs

2085485

fix the doc fixes

d6780c4

tthsqe12 commented Dec 2, 2020

View reviewed changes

this might be what tommy suggested

5e32787

rfourquet reviewed Dec 2, 2020

View reviewed changes

src/flint/fq.jl Outdated Show resolved Hide resolved

src/flint/fq.jl Outdated Show resolved Hide resolved

fingolfin mentioned this pull request Dec 4, 2020

Updating FLINT_jll (and other JLLs) across all OSCAR packages oscar-system/Oscar.jl#193

Closed

21 tasks

Update Project.toml

962da42

fingolfin reviewed Dec 11, 2020

View reviewed changes

Project.toml Outdated

@@ -24,7 +24,7 @@ AbstractAlgebra = "^0.11.1"

Antic_jll = "~0.2.2"

Arb_jll = "~2.18.1"

BinaryProvider = "0.4, 0.5"

FLINT_jll = "~2.6.3"

FLINT_jll = "~200.690.0"

LoadFlint = "^0.3.4"

This comment was marked as resolved.

Sign in to view

fingolfin reviewed Dec 11, 2020

View reviewed changes

thofma added 2 commits December 11, 2020 18:59

Small adjustments

7fad439

Adjust build.jl

8e67c2c

Fix FqFiniteField constructors

9966d45

Add Windows to github actions

c6e6db1

fingolfin approved these changes Dec 11, 2020

View reviewed changes

Merge branch 'master' into prep_flint2p7

a52c121

thofma and others added 2 commits December 15, 2020 11:30

Update src/flint/fq.jl

d5ded49

Co-authored-by: Rafael Fourquet <fourquet.rafael@gmail.com>

Update src/flint/fq.jl

26c9115

Co-authored-by: Rafael Fourquet <fourquet.rafael@gmail.com>

This comment has been minimized.

Sign in to view

fingolfin mentioned this pull request Dec 15, 2020

Revise libsingular_julia JuliaPackaging/Yggdrasil#1798

Merged

tthsqe12 changed the title ~~Prep flint 2.7~~ Prep flint 2.7: resolves #860, resolves #870 Dec 15, 2020

add finite field issquare_with_square_root

1bbb7d6

fingolfin mentioned this pull request Dec 16, 2020

libsingular_julia for Julia 1.4 JuliaPackaging/Yggdrasil#2263

Merged

thofma added a commit to thofma/Yggdrasil that referenced this pull request Dec 16, 2020

[FLINT] new version

5f405c6

@fingolfin `200.690` is too old to push Nemocas/Nemo.jl#919 over the finish line. Did I bump the version correctly? Or should I bump the patch version?

thofma mentioned this pull request Dec 16, 2020

[FLINT] new version JuliaPackaging/Yggdrasil#2265

Merged

Wrap can_solve

940ce9c

thofma added 2 commits December 19, 2020 15:00

Bump Antic version

55fc55e

Bump Arb version to 2.19.0

f827ce9

thofma closed this Dec 20, 2020

thofma reopened this Dec 20, 2020

Add build_FLINT_ file

5f625c2

Remove old build files

c94e078

thofma merged commit b2fef8f into Nemocas:master Dec 21, 2020

tthsqe12 deleted the prep_flint2p7 branch December 9, 2021 10:03

Prep flint 2.7: resolves #860, resolves #870 #919

Prep flint 2.7: resolves #860, resolves #870 #919

Conversation

tthsqe12 commented Nov 28, 2020 • edited Loading

thofma commented Nov 28, 2020

rfourquet Dec 1, 2020

Choose a reason for hiding this comment

rfourquet Dec 1, 2020

Choose a reason for hiding this comment

tthsqe12 Dec 2, 2020

Choose a reason for hiding this comment

thofma Dec 2, 2020

Choose a reason for hiding this comment

tthsqe12 Dec 2, 2020

Choose a reason for hiding this comment

thofma Dec 2, 2020

Choose a reason for hiding this comment

rfourquet Dec 2, 2020

Choose a reason for hiding this comment

thofma commented Dec 2, 2020 • edited Loading

This comment was marked as resolved.

thofma commented Dec 11, 2020 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

thofma commented Dec 11, 2020

thofma commented Dec 11, 2020

fingolfin commented Dec 11, 2020

thofma commented Dec 11, 2020

fingolfin commented Dec 11, 2020

thofma commented Dec 11, 2020

fingolfin commented Dec 11, 2020

fingolfin left a comment

Choose a reason for hiding this comment

tthsqe12 commented Dec 11, 2020

tthsqe12 commented Dec 12, 2020 • edited Loading

fingolfin commented Dec 15, 2020

This comment has been minimized.

thofma commented Dec 15, 2020

tthsqe12 commented Dec 15, 2020 • edited Loading

thofma commented Dec 15, 2020

fingolfin commented Dec 15, 2020

fingolfin commented Dec 15, 2020

thofma commented Dec 16, 2020

wbhart commented Dec 16, 2020

fingolfin commented Dec 21, 2020

thofma commented Dec 21, 2020

tthsqe12 commented Dec 21, 2020

tthsqe12 commented Nov 28, 2020 •

edited

Loading

thofma commented Dec 2, 2020 •

edited

Loading

thofma commented Dec 11, 2020 •

edited

Loading

tthsqe12 commented Dec 12, 2020 •

edited

Loading

tthsqe12 commented Dec 15, 2020 •

edited

Loading