Fix 740 #742

DiamonDinoia · 2025-10-24T14:57:38Z

@mreineck when building locally. The issue seems to be just in the test. Not in anything public facing.

mreineck · 2025-10-24T17:34:47Z

This looks good, but I still think that using plain abs() in a C++ code (as is happening in other places of finufft) is an accident waiting to happen. Including <math.h> is archaic C++ for 27 years now, and if things go wrong, they tend to go wrong very subtly and require tedious debugging.
It's happened to me, and I've seen it happen to several other people ... but this can definitely wait for a follow-up patch.

DiamonDinoia · 2025-10-24T17:59:53Z

@mreineck I totally agree. We should not use abs in .cpp code. The issue with std::abs is when used on thrust::complex

DiamonDinoia · 2025-10-24T18:13:13Z

I turned that warning in an error. To mitigate the issue. Let me try having a go at fixing this.

ahbarnett · 2025-10-24T18:19:13Z

When I include std:: in testutils, as in this new version, tests pass fine on my linux machine with GCC 11.4.
So, I don't understand the CI "illegal" errors.

Note that in some sources in test/ I have using namespace std; which is probably bad practice but saved a lot of typing of std:: for math.
I also used fabs in many places.

Happy to insert std:: everywhere needed.

mreineck · 2025-10-24T18:30:50Z

The "illegal" errors are unrelated, I'm quite sure. I just have no idea what is causing them.

lu1and10 · 2025-10-24T18:33:05Z

the CI "illegal" errors.

@ahbarnett I'm not sure why CI breaks so often. One possible reason is, to accelerate the ci runs, @DiamonDinoia uses sccache in ci to cache the compiled object files(.o) from previous ci runs, if the previous ci run was ran on avx512 machine with flag -march=native, and the next ci run re-uses the cached object files and run on avx2 machine, there might be illegal instructions.

What is the "key" that sccache used to distinguish changes(currently how sccache decide to reuse .o files or not), besides the C++ flags, does sscache distinguish cpu instructions?

Currently, it seems with "-march=native", the ci may break with reused object files, I guess.

mreineck · 2025-10-24T18:36:31Z

Oh! If files compiled with -march=native are re-used on different hardware, the "illegal" makes perfect sense ... it's a cut-down "illegal instruction" error.

But CI should definitely never do this ...

And "illegal instruction" should not be truncated like this ... strange,

lu1and10 · 2025-10-24T18:52:34Z

But CI should definitely never do this ...

Yes, not sure if it's sccache problem. If it is, we should tell it, machine changed, with -march=native, please rerun no reuse...

DiamonDinoia · 2025-10-24T19:18:18Z

But CI should definitely never do this ...

Yes, not sure if it's sccache problem. If it is, we should tell it, machine changed, with -march=native, please rerun no reuse...

Worth opening a separate issue for it. We could add the instruction set from lscpu to the key. Or manually set the vector width. This allows to test different vectorizations same as xsimd does.

PS: @lu1and10 that's a very good guess! I was wondering what the problem was too.

lu1and10 · 2025-10-24T19:24:23Z

We could add the instruction set from lscpu to the key.

Good idea, we can retain the ci speedup from sccache and at the same time avoid illegal instructions.

DiamonDinoia · 2025-10-28T15:13:51Z

@mreineck I went ahead and remove using namespace std and fixed all the compiler warning/errors that originates from it (-werror has be incredibly useful).

I also fixed examples and one documentation file containing using namespace std. I don't think we should encourage bad practices.

I ran this though an LLM to tell me if I missed something and it says it is okay.

@mreineck, @lu1and10 what do you think?

DiamonDinoia · 2025-10-28T15:17:40Z

@blackwer I am a confused by examples/cuda/getting_started.cpp it seems to me a c file masquerading as cpp.

mreineck · 2025-10-28T15:25:09Z

Hi @DiamonDinoia, this is of course the cleanest approach to fix the issue (and also prevent it from happening again)!

I wouldn't say that using namespace std; is bad practice in general; it's only unconditionally bad if you "force" it onto users by putting it unprotected in a header file. Apart from that it can save a lot of typing; you only need to make sure that you are not using any "C-like" headers (like <math.h>).

That said, I like the change very much as it is, even though it touches a lot of files!

mreineck · 2025-10-28T15:28:03Z

examples/cuda/example2d2many.cpp

    int m = 0;
    for (int m2 = -(N2 / 2); m2 <= (N2 - 1) / 2; ++m2) // loop in correct order over F
      for (int m1 = -(N1 / 2); m1 <= (N1 - 1) / 2; ++m1)
        ct += fkstart[m++] * exp(J * (m1 * x[jt] + m2 * y[jt])); // crude direct


Here, the exp seems to be missing a std::. Don't put too much trust in LLMs :)

grep was my best friend here...

blackwer · 2025-10-28T16:06:19Z

@blackwer I am a confused by examples/cuda/getting_started.cpp it seems to me a c file masquerading as cpp.

This whole package was written with as C plus some stuff. It's fine to update it to a more proper C++ style. It was just consistent with the way everything else was written.

ahbarnett

Thanks for doing this. It's good you enforced the correct abs and exp etc in the library source and tests.

I feel that it makes the example codes much less readable. I would prefer the examples to simply using namespace std; because they are so limited, short, and designed for humans to read (Would it be a pain to just git checkout the examples from master, and fix the header from math.h to cmath ?). Ie, I think the PR got a little too big in scope, beyond just making sure the src/tests were secure.

In fact the whole thing makes me depressed about C++ ... what's the point of a language where you have to write std:: in front of every other function? Declaring complex vectors is particularly ugly now, as is any stdio stuff. It's not like we are inserting using namespace std; into a header file that affects other people here. It is just there to make clean, short, source codes for testing and examples.

Thoughts?

ahbarnett · 2025-10-28T20:40:35Z

examples/guru1d1.cpp

-    c[j] =
-        2 * ((double)rand() / RAND_MAX) - 1 + 1i * (2 * ((double)rand() / RAND_MAX) - 1);
+    c[j] = 2 * ((double)std::rand() / RAND_MAX) - 1 +
+           std::complex<double>(0.0, 1.0) *


Can we use I which was defined above for a reason? I must say this std:: everywhere is highly unpleasant! I'm not sure why in our example codes we're killing using namespace std; ....

ahbarnett · 2025-10-28T20:40:47Z

examples/guru1d1.cpp

-    c[j] =
-        2 * ((double)rand() / RAND_MAX) - 1 + 1i * (2 * ((double)rand() / RAND_MAX) - 1);
+    c[j] = 2 * ((double)std::rand() / RAND_MAX) - 1 +
+           std::complex<double>(0.0, 1.0) *


ahbarnett · 2025-10-28T20:43:17Z

examples/guru1d1.cpp

-    x[j] = PI * (2 * ((double)rand() / RAND_MAX) - 1); // uniform random in [-pi,pi)
+    x[j] = PI * (2 * ((double)std::rand() / RAND_MAX) - 1); // uniform random in [-pi,pi)
  // note FINUFFT doesn't use std::vector types, so we need to make a pointer...
  finufft_setpts(plan, M, x.data(), NULL, NULL, 0, NULL, NULL, NULL);


nullptr ? Surely it doesn't matter since setpts ignores them.

I missed it. I'll fix it.

ahbarnett · 2025-10-28T20:43:43Z

examples/guru1d1f.cpp

+    x[j] = PI * (2 * ((float)std::rand() / RAND_MAX) - 1); // uniform random in [-pi,pi)
  // note FINUFFT doesn't use std::vector types, so we need to make a pointer...
-  finufftf_setpts(plan, M, &x[0], NULL, NULL, 0, NULL, NULL, NULL);
+  finufftf_setpts(plan, M, &x[0], nullptr, nullptr, 0, nullptr, nullptr, nullptr);


ahbarnett · 2025-10-28T20:44:52Z

examples/guru2d1.cpp

+  int ier = finufft_makeplan(type, dim, Ns, +1, ntrans, tol, &plan, nullptr);
  // step 2: send in M nonuniform points (just x, y in this case)...
-  finufft_setpts(plan, M, &x[0], &y[0], NULL, 0, NULL, NULL, NULL);
+  finufft_setpts(plan, M, &x[0], &y[0], nullptr, 0, nullptr, nullptr, nullptr);


x.data() etc

ahbarnett · 2025-10-28T21:14:02Z

perftest/spreadtestnd.cpp

@@ -1,26 +1,21 @@
 #include "finufft/finufft_utils.hpp"
 #include <finufft/spreadinterp.h>
 #include <finufft/test_defs.h>


It seems like adding using namespace std::printf etc to test_defs.h would make this a whole lot less ugly...

That's a good idea! We can also add I to test_defs.

ahbarnett · 2025-10-28T21:21:34Z

src/c_interface.cpp

 // public header
 #include <finufft.h>
 // private headers
+#include <algorithm>


Could I ask what from the algorithm library is used? Ah, max I guess..

ahbarnett · 2025-10-28T21:23:41Z

test/cuda/cufinufft1d_test.cu


-#include <finufft_common/common.h>
 #include <cufinufft.h>
+#include <finufft_common/common.h>


If this header ordering is important, please add a comment...

ahbarnett · 2025-10-28T21:25:25Z

test/dumbinputs.cpp

+    F[k] = std::sin((FLT)0.7 * k) + IMA * std::cos((FLT)0.3 * k); // set F for t2
  ier = FINUFFT1D2(M, x, c, +1, 0, N, F, &opts);
  if (ier != FINUFFT_WARN_EPS_TOO_SMALL) {
    printf("1d2 tol=0:\twrong err code %d\n", ier);


I'm assuming printf is fine here, thank god! Not sure why it wasn't fine earlier...

ahbarnett · 2025-10-28T21:27:15Z

test/finufft3d_test.cpp

  free(u);
-  if (isnan(errmax) || (errmax > errfail)) {
+  if (std::isnan(errmax) || (errmax > errfail)) {
    printf("\tfailed! err %.3g > errfail %.3g\n", errmax, errfail);


hoping printf is fine here... please :)

DiamonDinoia · 2025-10-28T21:46:49Z

In general I do not like using exp in c++ or abs without std:: because I never know if I am calling a compiler builtin with implicit conversion or an actual function. One way to make is safer is to use -Wconversion

DiamonDinoia requested review from ahbarnett and mreineck October 24, 2025 15:26

mreineck approved these changes Oct 24, 2025

View reviewed changes

DiamonDinoia force-pushed the fix/740 branch from cdee998 to 1702509 Compare October 28, 2025 15:05

mreineck reviewed Oct 28, 2025

View reviewed changes

Fix 740

7fbc700

DiamonDinoia force-pushed the fix/740 branch from 1702509 to 7fbc700 Compare October 28, 2025 18:18

ahbarnett requested changes Oct 28, 2025

View reviewed changes

Fix 740 #742

Are you sure you want to change the base?

Fix 740 #742

Uh oh!

Conversation

DiamonDinoia commented Oct 24, 2025

Uh oh!

mreineck commented Oct 24, 2025

Uh oh!

DiamonDinoia commented Oct 24, 2025

Uh oh!

DiamonDinoia commented Oct 24, 2025

Uh oh!

ahbarnett commented Oct 24, 2025

Uh oh!

mreineck commented Oct 24, 2025

Uh oh!

lu1and10 commented Oct 24, 2025

Uh oh!

mreineck commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lu1and10 commented Oct 24, 2025

Uh oh!

DiamonDinoia commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lu1and10 commented Oct 24, 2025

Uh oh!

DiamonDinoia commented Oct 28, 2025

Uh oh!

DiamonDinoia commented Oct 28, 2025

Uh oh!

mreineck commented Oct 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackwer commented Oct 28, 2025

Uh oh!

ahbarnett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiamonDinoia commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mreineck commented Oct 24, 2025 •

edited

Loading

DiamonDinoia commented Oct 24, 2025 •

edited

Loading

DiamonDinoia commented Oct 28, 2025 •

edited

Loading