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

Install the library using the corresponding JLL package #21

Merged
merged 1 commit into from Sep 8, 2020

Conversation

giordano
Copy link
Contributor

@giordano giordano commented Aug 2, 2020

Close #19

@coveralls
Copy link

coveralls commented Aug 2, 2020

Pull Request Test Coverage Report for Build 146

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 93.802%

Totals Coverage Status
Change from base Build 142: 0.3%
Covered Lines: 227
Relevant Lines: 242

💛 - Coveralls

@giordano
Copy link
Contributor Author

giordano commented Aug 2, 2020

Tests passing on all operating systems (including FreeBSD and Windows, never tested before) without having to install Conda nor any compiler 🙂

@ludvigak
Copy link
Owner

ludvigak commented Aug 8, 2020

Nice! I'm away from computer for a while, but will merge once I'm back and have tried it myself.

One question though, do we have to disable OpenMP, or could we maybe build FFTW with threading ourselves and ship it in the JLL?
It would really be a bummer to have FINUFFT run single core only in the Julia interface.

@giordano
Copy link
Contributor Author

giordano commented Aug 8, 2020

One question though, do we have to disable OpenMP, or could we maybe build FFTW with threading ourselves and ship it in the JLL?

I'm a bit confused by this: FFTW does have threading enabled, and it's used by FFTW.jl. Isn't it sufficient?

@ludvigak
Copy link
Owner

ludvigak commented Aug 8, 2020

I'm referring to the comment by @jonas-kr in #19, but maybe I should have a look at the code before I open my mouth.
In any case, FINUFFT does have a significantly portion of code that is accelerated by threading, outside of the FFT calls sent to FFTW.

@giordano
Copy link
Contributor Author

giordano commented Aug 8, 2020

Can't we simply enable OpenMP in finufft? I don't understand how FFTW is relevant

@jkrimmer
Copy link
Contributor

jkrimmer commented Aug 9, 2020

Actually, I have already created a custom build of the jll with openMP enabled! I do not have much time today, so I will provide an update tomorrow.

@jkrimmer
Copy link
Contributor

Pull request for updated finufft_jll is open and builds successfully on all platforms. The main disadvantage of enabled threading is that finufft uses all available threads by default (despite most x86 CPUs featuring SMT) by default, which can lead to worse performance than the single-threaded scenario. I hope that the new guruInterface_v2 branch is ready for release soon such that it becomes possible to configure the number of threads.

@MikaelSlevinsky
Copy link

I think you can get around the performance loss of hyper-threading by initializing the library with half the Sys.CPU_THREADS

https://github.com/JuliaApproximation/FastTransforms.jl/blob/c68fceb0169d7736a29672c278513bb018c4b608/src/libfasttransforms.jl#L13-L20

@ludvigak
Copy link
Owner

ludvigak commented Sep 8, 2020

Thanks for the work @giordano and @jonas-kr , it's pretty great to finally have this package dragged into the modern world of jll-packaging =)
Also, sorry for dragging my feet on the merge!

@MikaelSlevinsky @jonas-kr regarding the threading, I think that's mainly due to FINUFFT having been developed for large datasets, and I think/hope that we'll develop some better heuristics for choosing # of threads. Anyway, tinkering with Sys.CPU_THREADS in the interface would risk messing with any such heuristics developed upstream.

@ludvigak ludvigak merged commit dd63e02 into ludvigak:master Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide binary dependency with JLL package
5 participants