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 the upstream version of finufft #107

Merged
merged 8 commits into from
Sep 11, 2024
Merged

Update the upstream version of finufft #107

merged 8 commits into from
Sep 11, 2024

Conversation

dfm
Copy link
Collaborator

@dfm dfm commented Aug 30, 2024

No description provided.

@dfm dfm requested a review from lgarrison August 30, 2024 14:04
@dfm dfm force-pushed the update-upstream branch from 04500f9 to ea9cbca Compare August 30, 2024 14:08
@lgarrison
Copy link
Member

I think we probably want to compile the static library with -fPIC? I.e. target_compile_options(-fPIC PRIVATE finufft) in our CMake. Maybe using a finufft shared library instead would be better for codes that use both finufft and jax-finufft, but that's probably a corner case and requires care with rpath and auditwheel.

@lgarrison
Copy link
Member

It looks like we need to set the POSITION_INDEPENDENT_CODE CMake property on (1) the finufft libs and (2) their dependencies. I don't know of an elegant way to set this on (2), so for now I'm just doing it explicitly. This probably means we should ask the finufft folks to add a FINUFFT_POSITION_INDEPENDENT_CODE flag.

In any case, this seems to have pushed the MacOS error to a missing FFTW dep. Maybe we need an explicit FFTW dependency in our CMake? Not sure why Ubuntu would work in that case, maybe because it's installing the headers in system paths?

Copy link
Member

@lgarrison lgarrison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Although we might want to wait until finufft 2.3 lands, which should be in the next week.

@lgarrison
Copy link
Member

finufft 2.3 is out! I updated the vendored dependency here, and things look good. Ready to merge?

@dfm dfm merged commit 40a8d68 into main Sep 11, 2024
6 checks passed
@dfm dfm deleted the update-upstream branch September 11, 2024 23:51
@dfm
Copy link
Collaborator Author

dfm commented Sep 11, 2024

Thanks!!

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.

2 participants