Skip to content

Conversation

@DiamonDinoia
Copy link
Collaborator

No description provided.

@DiamonDinoia DiamonDinoia force-pushed the add-kernel-selection-opts branch 4 times, most recently from 5cff7c3 to d6cada9 Compare December 10, 2025 10:19
@DiamonDinoia
Copy link
Collaborator Author

DiamonDinoia commented Dec 10, 2025

The test now catches the problem spotted by @lu1and10 : https://github.com/flatironinstitute/finufft/actions/runs/20095171541/job/57652014814?pr=763

11/23 Test #11: run_accuracy_test_float ..........***Failed    0.39 sec
tol=1.000000e-01 req=2.000000e-01 rel_err=1.046724e+00 -> FAILED (decade=1e-1)

@lu1and10
Copy link
Member

very nice, should merge this once master fixed.

@DiamonDinoia DiamonDinoia force-pushed the add-kernel-selection-opts branch from d6cada9 to d63fac0 Compare December 10, 2025 16:37
@DiamonDinoia DiamonDinoia force-pushed the add-kernel-selection-opts branch from d63fac0 to dbca829 Compare December 10, 2025 16:38
@DiamonDinoia DiamonDinoia force-pushed the add-kernel-selection-opts branch from 95ba004 to e7fb118 Compare December 10, 2025 17:28
@DiamonDinoia
Copy link
Collaborator Author

@lu1and10 the failure in the test might have been another issue that it took me a while to spot.

I think this is ready for review now.

@lu1and10
Copy link
Member

by another issue, do you mean ns for upsampfac 2.0 in float can't be 2?
if (std::is_same_v<T, float> && upsampfac == 2.00) ns = std::max(ns, 3);

@DiamonDinoia DiamonDinoia force-pushed the add-kernel-selection-opts branch from e7fb118 to 404fd89 Compare December 10, 2025 17:36
@DiamonDinoia
Copy link
Collaborator Author

DiamonDinoia commented Dec 10, 2025

I moved kernel functions in kernel.h kernel.cpp make a static_library for finufft and cufinufft. A bit of cleanup.

Reason why I moved the kernel functions there is that I can link the test against this library and clamp eps to the max_eps allowed per sigma. This function takes kernel_type as input so we can return something different for different kernels.

by another issue, do you mean ns for upsampfac 2.0 in float can't be 2? if (std::is_same_v<T, float> && upsampfac == 2.00) ns = std::max(ns, 3);

yes, if you keep ns = 2 in that case it gives 0(1) errors for tol exactly 1e-1

@DiamonDinoia DiamonDinoia force-pushed the add-kernel-selection-opts branch from 404fd89 to 8214f38 Compare December 10, 2025 17:44
@DiamonDinoia
Copy link
Collaborator Author

I noticed that there are again formatting changes. I will revert them tomorrow. (I'll have time hopefully)

@lu1and10
Copy link
Member

I noticed that there are again formatting changes. I will revert them tomorrow. (I'll have time hopefully)

No worries, it's not urgent and I can try to help with it if you don't have time.

@lu1and10
Copy link
Member

@DiamonDinoia I guess the format is not a problem for this PR, if you run clang-format -i -style=file src/*.cpp in master, there are some .cpp files that haven't been formatted for a while since the cpp was missing in the .pre-commit-config.yaml.
Maybe we could just make a single commit only to format the src/*.cpp files in master and put it in .git-blame-ignore-revs, what do you think.

@DiamonDinoia
Copy link
Collaborator Author

DiamonDinoia commented Dec 10, 2025 via email

@DiamonDinoia DiamonDinoia force-pushed the add-kernel-selection-opts branch from 8214f38 to 6b06e91 Compare December 10, 2025 21:36
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