Skip to content
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

Update FLINT_jll #1568

Merged
merged 5 commits into from
May 4, 2024
Merged

Update FLINT_jll #1568

merged 5 commits into from
May 4, 2024

Conversation

albinahlback
Copy link
Contributor

@albinahlback albinahlback commented Oct 30, 2023

And remove dependencies of Arb_jll, Antic_jll and Calcium_jll.

Solves #1567

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Attention: Patch coverage is 83.98844% with 277 lines in your changes are missing coverage. Please review.

Project coverage is 84.93%. Comparing base (f102e74) to head (3e04526).
Report is 1 commits behind head on master.

Files Patch % Lines
src/arb/ArbTypes.jl 53.88% 89 Missing ⚠️
src/arb/Complex.jl 87.42% 21 Missing ⚠️
src/arb/acb.jl 87.27% 21 Missing ⚠️
src/HeckeMoreStuff.jl 0.00% 18 Missing ⚠️
src/arb/acb_poly.jl 76.27% 14 Missing ⚠️
src/antic/nf_elem.jl 80.88% 13 Missing ⚠️
src/arb/ComplexPoly.jl 78.68% 13 Missing ⚠️
src/flint/FlintTypes.jl 80.64% 12 Missing ⚠️
src/arb/RealPoly.jl 78.00% 11 Missing ⚠️
src/arb/ComplexMat.jl 81.48% 10 Missing ⚠️
... and 8 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1568      +/-   ##
==========================================
+ Coverage   84.90%   84.93%   +0.03%     
==========================================
  Files          95       95              
  Lines       37255    37250       -5     
==========================================
+ Hits        31631    31638       +7     
+ Misses       5624     5612      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thofma
Copy link
Member

thofma commented Oct 30, 2023

Eventually we will do this, but we first need to rebuild all binaries in Oscar that link against flint to use 3.0.0. So we have to put this on hold here for the moment.

@thofma
Copy link
Member

thofma commented Oct 31, 2023

We could merge this in a branch and let flint test against that branch. What do you think?

@albinahlback
Copy link
Contributor Author

We could merge this in a branch and let flint test against that branch. What do you think?

What's the time frame of the new binaries for Oscar? In the mean time we can just test against my branch in my repo.

@thofma
Copy link
Member

thofma commented Oct 31, 2023

There is no real urgency on our side, since there is nothing new in 3.0. So maybe weeks, months?

@albinahlback
Copy link
Contributor Author

Just to note it here: Before any merging is done, flintlib/flint#1719 would have to be solved.

@thofma
Copy link
Member

thofma commented Jan 18, 2024

is it complicated to fix?

@albinahlback
Copy link
Contributor Author

is it complicated to fix?

No, not really. We align by pages, so we either have to use the user-assigned malloc to allocate some more memory than needed (don't know how much we typically allocate), or we have to scrap aligning by pages if user has set the memory functions.

@albinahlback
Copy link
Contributor Author

So FLINT 3.0 should not be used with Nemo due to the GC problem, but the upcoming 3.1 should work!

@fingolfin
Copy link
Member

It seems FLINT 3.1 is now out and in a JLL, and as far as I know, this is now supposed to be useable within OSCAR.

Unfortunately this PR now has many conflicts. @albinahlback would you be willing to update it?

@albinahlback
Copy link
Contributor Author

It seems FLINT 3.1 is now out and in a JLL, and as far as I know, this is now supposed to be useable within OSCAR.

Unfortunately this PR now has many conflicts. @albinahlback would you be willing to update it?

Sorry, I completely missed this. Will do.

@albinahlback
Copy link
Contributor Author

albinahlback commented Apr 24, 2024

Hopefully this works.

Edit: There were some new(?) functions that did not push any reference to the contexts, should hopefully be fixed now.

@albinahlback
Copy link
Contributor Author

@thofma can you check that the fix I made is correct according to the GC?

@albinahlback
Copy link
Contributor Author

Btw, how do I run a specific test in Nemo?

@albinahlback
Copy link
Contributor Author

Seems to work fine. Let me know if it is and I will squash the commits.

src/flint/fq_nmod.jl Outdated Show resolved Hide resolved
@thofma
Copy link
Member

thofma commented Apr 25, 2024

Looks good, thanks

@thofma
Copy link
Member

thofma commented Apr 25, 2024

Btw, how do I run a specific test in Nemo?

Once upon a time, one could do something like

Nemo.test_module("flint", "fmpz")

but this is broken.

@fingolfin
Copy link
Member

fingolfin commented Apr 25, 2024

I took the liberty of splitting this into two commits: one with the "actual" changes, and a second one which takes care of replacing libantic, libarb, libcalcium by libflint. The second commit was made by modifying src/Nemo.jl manually and then doing

git grep -l ccall src test | xargs perl -pi -e 's;lib(antic|arb|calcium);libflint;'

This way it is much easier to see what the "real" changes are.

@albinahlback albinahlback marked this pull request as ready for review April 25, 2024 12:26
@thofma
Copy link
Member

thofma commented Apr 25, 2024

I am trying this with Hecke. It is not looking good. @albinahlback any hint what Impossible conversion could mean in

     From worker 3:	Flint exception (General error):
      From worker 3:	    Impossible conversion
      From worker 3:
      From worker 3:	[16981] signal (11.2): Segmentation fault: 11
      From worker 3:	in expression starting at
[...]
      From worker 3:	_fq_nmod_vec_clear at /Users/bla/artifacts/fd3613cc70f27c578eab3cfd080ce8edcb3aba44/lib/libflint.19.0.dylib (unknown line)
      From worker 3:	_fq_nmod_poly_clear_fn at /Users/bla/dev/Nemo/src/flint/FlintTypes.jl:6117

@thofma
Copy link
Member

thofma commented Apr 25, 2024

I think I found out what the problem is, but it would still be good to know what the "Impossible conversion" hints at.

@albinahlback
Copy link
Contributor Author

I will try my best to fix this. But why doesn't this show up in the tests for Nemo?

@albinahlback
Copy link
Contributor Author

I don't think there is anything that needs to be changed in order to work? I will just make fq_default_ctx_struct a little bit smaller in Nemo as it is now equivalent to gr_ctx_struct.

@thofma
Copy link
Member

thofma commented Apr 25, 2024

The tests are not that extensive. I reduced it to the following change in behavior:

Before:

julia> R = Native.GF(7)
Finite field of characteristic 7

julia> Rx, x = R["x"]
(Univariate polynomial ring in x over GF(7), x)

julia> FqField(x^2 + 1, :asds) |> Nemo._fq_default_ctx_type
2 # corresponds to fq_nmod

With the PR here:

julia> R = Native.GF(7)
Finite field of characteristic 7

julia> Rx, x = R["x"]
(Univariate polynomial ring in x over GF(7), x)

julia> FqField(x^2 + 1, :asds) |> Nemo._fq_default_ctx_type
3 # corresponds to fq

Note that x is an fpPolyRingElem, which is an alias for nmod_poly. If I feed in an nmod_poly, it should construct an fq_nmod disguised as fq_default.

albinahlback and others added 2 commits April 25, 2024 16:10
This includes:

* Reduce size of fq_default_ctx_struct to match the synonymous
  gr_ctx_struct.
* Adjust for FLINT 3.1 fq_nmod where ulongs are now encouraged within
  this module.
* Adjust for FLINT 3.1 fmpz_mod_mat changes where context objects should
  be part of the argument.
@albinahlback
Copy link
Contributor Author

@fredrik-johansson do you know why this is?

@thofma
Copy link
Member

thofma commented Apr 25, 2024

P.S.: This is just calling fq_default_ctx_init_modulus_nmod with a plain nmod_poly of degree 2.

@thofma
Copy link
Member

thofma commented Apr 25, 2024

Next error/question: What is the recommended substitute for nmod_poly_powmod_mpz_binexp? The history.rst file just told me that is was removed, but not what I should use instead.

@albinahlback
Copy link
Contributor Author

Next error/question: What is the recommended substitute for nmod_poly_powmod_mpz_binexp? The history.rst file just told me that is was removed, but not what I should use instead.

nmod_poly_powmod_fmpz_binexp should work. I have tried to remove functions with mpz inputs as that just create duplicate code, and we really want to work with just fmpz.

@thofma
Copy link
Member

thofma commented Apr 25, 2024

Thanks

src/flint/fq_nmod.jl Outdated Show resolved Hide resolved
@thofma
Copy link
Member

thofma commented Apr 25, 2024

Thanks!

Co-authored-by: Tommy Hofmann <[email protected]>
@albinahlback
Copy link
Contributor Author

BTW, we will have to push a new patch for this you found, i.e. v3.1.3. (Just one more patch until vπ)

@thofma
Copy link
Member

thofma commented Apr 25, 2024

Hm, what happend to fmpz_mod_poly_inv_series_newton?

@albinahlback
Copy link
Contributor Author

Fredrik rewrote some code to utilize GR instead. I think it should be replaced with fmpz_mod_poly_inv_series, which then chooses what algorithm to use in the GR module.

@thofma
Copy link
Member

thofma commented Apr 25, 2024

I see, thanks

@albinahlback
Copy link
Contributor Author

Who should I contact for FLINT_jll? Will soon create 3.1.3.

@albinahlback
Copy link
Contributor Author

I created v3.1.3. Please check it out if you have time.

@thofma
Copy link
Member

thofma commented Apr 26, 2024

Yes, thanks, I can confirm that the fq_default problems have been fixed. I triggered building the new binaries here: JuliaPackaging/Yggdrasil#8536

But even with this fix, we are still experiencing segfaults in the Hecke tests, which we are investigating. Not sure yet whose fault it is.

@albinahlback
Copy link
Contributor Author

What versions of each package should I use to check? Nemo and Hecke have different versions required for AA

@albinahlback
Copy link
Contributor Author

I can't get the testing of Hecke to work at all on my system.

@thofma
Copy link
Member

thofma commented Apr 26, 2024

There is no need for you to look into it. I will let you know if I find something related.

@thofma thofma mentioned this pull request May 4, 2024
11 tasks
@thofma thofma merged commit e76cdb3 into Nemocas:master May 4, 2024
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants