NUFFT + Installation fix

[ericcanton]

• Adds Non-Uniform FFT method, to allow dynamic rate spectrograms without hard resampling first.
• Updates to CMakeLists so that FINUFFT installs automatically and correctly.
• Updates to pip package to make this install more dependable.

[Copilot]

Pull request overview

This PR introduces FINUFFT-based non-uniform FFT support to compute spectrograms directly from irregularly sampled signals (avoiding time-domain resampling artifacts), adds a cubic-spline resampler, and updates the native/Python build paths to make FINUFFT integration easier to install and use.

Changes:

• Add C++ NUFFT spectrogram computation (FINUFFT) plus Python bindings, and add cubic-spline accelerometer resampling.
• Update build configuration (top-level CMake + senpy/setup.py/pyproject.toml + senpy/CMakeLists.txt) to build/link FINUFFT and improve install reliability.
• Add artifact/benchmark-style tests and a plotting script to compare linear vs cubic vs NUFFT-direct pipelines.

Reviewed changes

Copilot reviewed 10 out of 12 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
src/sensor_processing_native.cpp	Adds FINUFFT NUFFT spectrogram, cubic spline helpers/resampler, and pybind11 exports.
senpy/api.py	Exposes resample_accelerometer_cubic* and compute_spectrogram_nufft in the Python API.
senpy/setup.py	Builds FINUFFT artifacts via top-level CMake and links/copies runtime libs into the wheel.
senpy/pyproject.toml	Updates build-system requirements (adds numpy).
senpy/CMakeLists.txt	Adds optional FINUFFT include/link wiring for building _core via CMake.
CMakeLists.txt	Updates compiler flags and fetches FINUFFT via CPM; links it into the native library.
senpy/tests/test_resampling_artifacts.py	Adds tests/metrics comparing artifact behavior across methods.
senpy/tests/conftest.py	Adds fixtures to load real CSV datasets for the new tests.
senpy/plot_spectrogram_comparison.py	Adds a script to generate visual spectrogram comparisons across methods.
finufft_migration_plan.md	Adds a written plan for making FINUFFT dependency self-contained.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

finufft.h is included unconditionally, but some build paths (e.g. senpy/CMakeLists.txt when USE_FINUFFT is OFF) explicitly support building without FINUFFT. As-is, builds without FINUFFT will fail at compile time. Please guard the include and any FINUFFT-dependent code with #ifdef USE_FINUFFT (or make FINUFFT a hard requirement everywhere and remove the conditional build path).

Suggested change

	#include <finufft.h>
	#ifdef USE_FINUFFT
	#include <finufft.h>
	#endif

[ericcanton]

ah, I actually want to go the other way: completely remove USE_FINUFFT which was not desireable to add. FINUFFT is a required package, as indicated by this unconditional import.

[Copilot]

computeSpectrogramNUFFT can crash or hang for small/invalid inputs: if timestamps.size() < 2, diffs is empty and diffs[diffs.size()/2] is undefined; and if secoverlap >= secperseg, hop_dur <= 0 leads to an infinite loop. Add explicit validation for minimum length, secperseg > 0, and 0 <= secoverlap < secperseg (and consider validating dt_median > 0).

[Copilot]

This windowing loop re-scans the entire timestamps array for every window, making the algorithm O(N * n_windows) and very slow for long recordings. Since timestamps are (presumably) sorted, maintain a moving start/end index (two-pointer approach) to select window samples in O(N + n_windows) total, or precompute per-window index ranges.

Suggested change

	while (win_start + win_dur <= t_end + dt_median)
	{
	double win_end = win_start + win_dur;
	// Select samples in window
	std::vector<double> t_win, s_win;
	for (size_t i = 0; i < timestamps.size(); ++i)
	{
	if (timestamps[i] >= win_start && timestamps[i] < win_end)
	{
	t_win.push_back(timestamps[i]);
	s_win.push_back(signal[i]);
	}
	// Use two-pointer indices to avoid rescanning timestamps for every window
	size_t start_idx = 0;
	size_t end_idx = 0;
	const size_t n = timestamps.size();
	while (win_start + win_dur <= t_end + dt_median)
	{
	if (start_idx >= n)
	{
	// No more samples available for subsequent windows
	break;
	}
	double win_end = win_start + win_dur;
	// Advance start_idx to the first sample >= win_start
	while (start_idx < n && timestamps[start_idx] < win_start)
	{
	++start_idx;
	}
	// Ensure end_idx is at least start_idx, then advance to first sample >= win_end
	if (end_idx < start_idx)
	{
	end_idx = start_idx;
	}
	while (end_idx < n && timestamps[end_idx] < win_end)
	{
	++end_idx;
	}
	// Select samples in window [start_idx, end_idx)
	std::vector<double> t_win, s_win;
	t_win.reserve(end_idx > start_idx ? (end_idx - start_idx) : 0);
	s_win.reserve(end_idx > start_idx ? (end_idx - start_idx) : 0);
	for (size_t i = start_idx; i < end_idx; ++i)
	{
	t_win.push_back(timestamps[i]);
	s_win.push_back(signal[i]);

[Copilot]

If finufft_setpts or finufft_execute fails, this throws without destroying the created plan, leaking FINUFFT resources. Wrap the plan in RAII (e.g. unique_ptr with custom deleter) or ensure finufft_destroy(plan) is executed on all error paths (including after a successful makeplan but failed setpts/execute).

[Copilot]

The positive-frequency extraction omits the Nyquist bin (n_pos_freqs = nfft_padded/2), so freqs stops at fs/2 - 1/win_dur. When target_fs resampling is enabled, the target grid includes fmax=target_fs/2, which is beyond the last original frequency and will be clamped to the last value by evaluateCubicSpline, biasing the high end. Consider including the Nyquist-equivalent coefficient (FINUFFT mode -nfft/2, typically fhat_complex[0]) so the frequency axis reaches fs/2, or cap the target grid to the original max frequency.

[Copilot]

setup.py invokes the cmake CLI during build, but pyproject.toml doesn’t declare cmake as a build-system requirement. In PEP 517 isolated builds (e.g. pip install), this can fail even if users don’t have system CMake installed. Add an explicit cmake (and likely ninja/build tooling if required) dependency under [build-system].requires, or avoid shelling out to CMake during the Python build.

Suggested change

	requires = ["setuptools>=42", "wheel", "pybind11>=2.6", "numpy>=1.19.0"]
	requires = ["setuptools>=42", "wheel", "pybind11>=2.6", "numpy>=1.19.0", "cmake>=3.15", "ninja"]

[Copilot]

package_data={'senpy': ['*.so']} won’t include the copied runtime library on macOS (.dylib) or Windows (.dll), so wheels may be missing libfinufft at runtime. Consider including platform-specific patterns (e.g. *.so, *.dylib, *.dll) or using setuptools machinery to ensure the copied library is packaged.

Suggested change

	package_data={'senpy': ['*.so']},
	package_data={'senpy': ['*.so', '*.dylib', '*.dll']},

[Copilot]

This CMake file hardcodes FINUFFT/FFTW paths under /tmp/finufft, which is not portable and conflicts with the PR’s goal of making installation dependable. Prefer fetching/building FINUFFT as part of the CMake build (e.g. CPM/FetchContent like the top-level CMakeLists.txt) or allow the paths to be provided via cache variables / find_package instead of hardcoding.

Suggested change

	set(FINUFFT_INCLUDE_DIR "/tmp/finufft/include")
	set(FINUFFT_LIB "/tmp/finufft/build/src/libfinufft.a")
	set(FFTW3_LIB "/tmp/finufft/build/_deps/fftw3-build/libfftw3.a")
	set(FFTW3F_LIB "/tmp/finufft/build/_deps/fftw3f-build/libfftw3f.a")
	# These paths can be provided by the user or a parent CMake project.
	set(FINUFFT_INCLUDE_DIR "" CACHE PATH "Path to FINUFFT headers")
	set(FINUFFT_LIB "" CACHE FILEPATH "Path to the FINUFFT static library")
	set(FFTW3_LIB "" CACHE FILEPATH "Path to the FFTW3 static library")
	set(FFTW3F_LIB "" CACHE FILEPATH "Path to the FFTW3F static library")

[Copilot]

On Apple platforms this unconditionally runs brew --prefix libomp. If Homebrew isn’t installed (common on CI or non-brew setups), configuration will fail. Consider guarding with find_program(brew) and/or using find_package(OpenMP) first, only falling back to Homebrew-specific hints when needed.

Suggested change

	execute_process(COMMAND brew --prefix libomp OUTPUT_VARIABLE LIBOMP_PREFIX OUTPUT_STRIP_TRAILING_WHITESPACE)
	set(OpenMP_C_FLAGS "-Xpreprocessor;-fopenmp;-I${LIBOMP_PREFIX}/include")
	set(OpenMP_CXX_FLAGS "-Xpreprocessor;-fopenmp;-I${LIBOMP_PREFIX}/include")
	set(OpenMP_C_LIB_NAMES "omp")
	set(OpenMP_CXX_LIB_NAMES "omp")
	set(OpenMP_omp_LIBRARY "${LIBOMP_PREFIX}/lib/libomp.dylib")
	# Only use Homebrew-specific hints if Homebrew is available.
	find_program(HOMEBREW_EXECUTABLE brew)
	if(HOMEBREW_EXECUTABLE)
	execute_process(
	COMMAND "${HOMEBREW_EXECUTABLE}" --prefix libomp
	OUTPUT_VARIABLE LIBOMP_PREFIX
	OUTPUT_STRIP_TRAILING_WHITESPACE
	)
	set(OpenMP_C_FLAGS "-Xpreprocessor;-fopenmp;-I${LIBOMP_PREFIX}/include")
	set(OpenMP_CXX_FLAGS "-Xpreprocessor;-fopenmp;-I${LIBOMP_PREFIX}/include")
	set(OpenMP_C_LIB_NAMES "omp")
	set(OpenMP_CXX_LIB_NAMES "omp")
	set(OpenMP_omp_LIBRARY "${LIBOMP_PREFIX}/lib/libomp.dylib")
	endif()

[Copilot]

This migration plan claims senpy/setup.py hardcodes /tmp/finufft/ paths and that pyproject.toml adds cmake to build requirements, but the actual code in this PR differs (setup.py builds via top-level CMake into ../build, and pyproject.toml currently omits cmake). Please update the document to match the implemented approach so future maintainers aren’t misled.

Suggested change

	`senpy/setup.py` hardcodes `/tmp/finufft/` paths and silently builds without NUFFT support if those paths don't exist. Getting a working build requires undocumented manual steps (clone finufft, run cmake with specific flags, build). This caused extensive debugging of the Dockerfile. The fix: make `setup.py` auto-fetch and build finufft if it's not present, so `pip install senpy` just works.
	**Constraint**: finufft must link dynamically (shared `.so`). Static linking causes undefined symbol errors at import time. Use `-DFINUFFT_STATIC_LINKING=OFF`.
	## Critical Files
	- [senpy/setup.py](../sensor_preprocessing_cpp/senpy/setup.py) — core rewrite
	- [senpy/pyproject.toml](../sensor_preprocessing_cpp/senpy/pyproject.toml) — add cmake to build deps
	- [senpy/.gitignore](../sensor_preprocessing_cpp/senpy/.gitignore) — add `_deps/`
	- [Dockerfile](Dockerfile) — remove manual finufft build block
	Before this work, `senpy/setup.py` relied on hardcoded `/tmp/finufft/` paths and could silently build without NUFFT support if those paths didn't exist, requiring undocumented manual steps (clone finufft, run cmake with specific flags, build) and causing extensive Dockerfile debugging. The implementation in this PR instead drives the existing top-level CMake configuration to build finufft into `../build`, avoiding `/tmp`-specific paths while keeping the build self-contained from a user perspective.
	**Constraint**: finufft must link dynamically (shared `.so`). Static linking causes undefined symbol errors at import time. Use `-DFINUFFT_STATIC_LINKING=OFF`.
	> Note: The detailed `_deps/finufft-…` auto-fetch flow described below was the original design; the current PR implementation builds finufft via the top-level CMake project into `../build` and assumes finufft sources are already present. Treat the auto-fetch logic as a future enhancement rather than a description of the current code.
	## Critical Files
	- [senpy/setup.py](../sensor_preprocessing_cpp/senpy/setup.py) — updated to invoke the top-level CMake build (outputting to `../build`) instead of using hardcoded `/tmp/finufft/` paths
	- [senpy/pyproject.toml](../sensor_preprocessing_cpp/senpy/pyproject.toml) — currently unchanged with respect to build dependencies (does **not** add `cmake`); consider adding `cmake` here in a follow-up if we want to enforce it as a build requirement
	- [senpy/.gitignore](../sensor_preprocessing_cpp/senpy/.gitignore) — add `_deps/` if/when the auto-fetch flow is implemented
	- [Dockerfile](Dockerfile) — remove manual finufft build block so it relies on the `setup.py`/top-level CMake integration