Skip to content

Conversation

@jenskeiner
Copy link
Contributor

@jenskeiner jenskeiner commented Sep 19, 2025

This PR adds a new workflow that runs the build on macOS.

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 19, 2025

CodSpeed Performance Report

Merging #168 will not alter performance

Comparing feature/macos-build (3b83060) with develop (8bec298)

Summary

✅ 88 untouched
🗄️ 154 archived benchmarks run1

Footnotes

  1. 154 benchmarks were run, but are now archived. If they were deleted in another branch, consider rebasing to remove them from the report. Instead if they were added back, click here to restore them.

@jenskeiner jenskeiner force-pushed the feature/macos-build branch 2 times, most recently from 2dd49e8 to 0c1a790 Compare September 23, 2025 13:04
@jenskeiner jenskeiner added the chore Repository maintenance and tooling changes label Sep 24, 2025
@jenskeiner jenskeiner changed the title Feature/macos build Add macOS build workflow. Sep 25, 2025
@jenskeiner jenskeiner force-pushed the feature/macos-build branch 3 times, most recently from 838b6ce to 29eb920 Compare September 26, 2025 07:04
@jenskeiner
Copy link
Contributor Author

jenskeiner commented Sep 26, 2025

Had to adjust one error bound slightly. Notably, long double on macOS/arm64 is the same as double, at least when using Apple's Clang or GCC. It's not even extended precision like on x86_64.

@jenskeiner jenskeiner force-pushed the feature/macos-build branch 2 times, most recently from bdf8211 to 306e856 Compare September 26, 2025 09:54
@jenskeiner jenskeiner marked this pull request as ready for review September 26, 2025 10:55
@jenskeiner
Copy link
Contributor Author

Had to de-activate the Codspeed benchmarks for macOS for now since building the benchmark library does not work on this platform yet.

@jenskeiner jenskeiner force-pushed the feature/macos-build branch 2 times, most recently from c0563a5 to 14d53ed Compare September 26, 2025 13:09
@jenskeiner jenskeiner force-pushed the feature/macos-build branch 5 times, most recently from 4c5f60a to 6410e92 Compare September 27, 2025 19:12
@michaelquellmalz
Copy link
Member

AFIK, x86 are the only current platforms offering the 80bit extended precision. Also for some compilers (such as Microsoft) long double is the same as double. Therefore using the MANT_DIG for distinguishing is good.

Is there a good reason that the new code insists on having certain MANT_DIG and throws errors instead? That means that one could not even compile NFFT on some exotic platforms. I would suggest to replace #elif LDBL_MANT_DIG >= 64 by #elif LDBL_MANT_DIG == 64 and similar. At least some implementations have different MANT_DIG, like certain PowerPCs use double-double arithmetics (I remember because of this we switched from EPS to MANT_DIG).

@jenskeiner jenskeiner added test Test-related changes with no production code impact and removed chore Repository maintenance and tooling changes labels Sep 29, 2025
@michaelquellmalz
Copy link
Member

Corresponding to my previous post, arm has a hardware implementation only of double. However, it seems like for arm64 (gcc-linux) long double is actually a software implementation of quadruple precision. This would explain why the tests (make check) take 1 hour in ubuntu-24.04-arm-gcc-kaiserbessel-long-double-openmp, but only 1 minute in ubuntu-24.04-arm-gcc-kaiserbessel-double-openmp.

@jenskeiner
Copy link
Contributor Author

AFIK, x86 are the only current platforms offering the 80bit extended precision. Also for some compilers (such as Microsoft) long double is the same as double. Therefore using the MANT_DIG for distinguishing is good.

Is there a good reason that the new code insists on having certain MANT_DIG and throws errors instead? That means that one could not even compile NFFT on some exotic platforms. I would suggest to replace #elif LDBL_MANT_DIG >= 64 by #elif LDBL_MANT_DIG == 64 and similar. At least some implementations have different MANT_DIG, like certain PowerPCs use double-double arithmetics (I remember because of this we switched from EPS to MANT_DIG).

Good point. I will double-check where we have code conditional on MANT_DIG (or still the floating-point type name). I am fine with still compiling everything, but maybe we should at least emit a warning where we see an unexpected MANT_DIG value.

@jenskeiner
Copy link
Contributor Author

Corresponding to my previous post, arm has a hardware implementation only of double. However, it seems like for arm64 (gcc-linux) long double is actually a software implementation of quadruple precision. This would explain why the tests (make check) take 1 hour in ubuntu-24.04-arm-gcc-kaiserbessel-long-double-openmp, but only 1 minute in ubuntu-24.04-arm-gcc-kaiserbessel-double-openmp.

Yes, correct. When using Apple's GCC, long double is the same as double. If you use upstream compiler versions, then "long double" is a quadruple precision data type. I have noticed that this runs quite slow, so was wondering if we should run a reduced set of benchmarks and tests then.

@jenskeiner
Copy link
Contributor Author

I am still observing occasional failures when running make check although the tests use a fixed seed for the random number generator. To facilitate debugging, I have added workflow steps that run the tests again if they have failed, and pipe the output to log files that are then uploaded as artifacts. But this only happens if the tests have failed in the first place. Unfortunately, make check does not pipe the output from the test programs anywhere so we have to run them again and assume they will run into the same issue again. Longer term, we should consider using a custom make target to compile, link and run the tests so that the output gets piped to files directly.

Otherwise, I fixed an issue where some tests were writing their output to stderr instead of stdout.

Lastly, I have adjusted all code sections that rely on MANT_DIG to assume double precision for an unexpected value. This way, the code will still compile, but breaks in tests and elsewhere could occur due to the code now possibly using a wrong assumption. I refrained from using #warning or similar preprocessor directives as they may not be really portable. I have also added a check to the configure script that will determine MANT_DIG as log a warning if the value is not any of the expected ones. This way the user will get a warning early on before compiling.

One last thought: It may make sense to try and remove some stuff from innft.h to a new header like nfft3util.h that also gets installed. Background: Our example programs, tests, and benchmarks us some definitions from infft.h that purely serve to abstract from the floating-point type used. This may also be useful for people building against the library, e.g. writing programs that can be configured with different floating-point types. This would also make it clearer that what remains in infft.h is strictly for internal use. I would hope that any test programs et cetera would no longer need to include infft.h and could just use nfft3util.h.

@michaelquellmalz: Maybe you can have a look again.

@tvolkmer
Copy link
Contributor

One last thought: It may make sense to try and remove some stuff from innft.h to a new header like nfft3util.h that also gets installed. Background: Our example programs, tests, and benchmarks us some definitions from infft.h that purely serve to abstract from the floating-point type used. This may also be useful for people building against the library, e.g. writing programs that can be configured with different floating-point types. This would also make it clearer that what remains in infft.h is strictly for internal use. I would hope that any test programs et cetera would no longer need to include infft.h and could just use nfft3util.h.

@jenskeiner The header nfft3mp.h was added for supporting different floating point types and is used in several simple_test examples. nfft3mp.h should already be installed.

@jenskeiner
Copy link
Contributor Author

One last thought: It may make sense to try and remove some stuff from innft.h to a new header like nfft3util.h that also gets installed. Background: Our example programs, tests, and benchmarks us some definitions from infft.h that purely serve to abstract from the floating-point type used. This may also be useful for people building against the library, e.g. writing programs that can be configured with different floating-point types. This would also make it clearer that what remains in infft.h is strictly for internal use. I would hope that any test programs et cetera would no longer need to include infft.h and could just use nfft3util.h.

@jenskeiner The header nfft3mp.h was added for supporting different floating point types and is used in several simple_test examples. nfft3mp.h should already be installed.

Ah, right, I need to take a look at it. In any case, infft.h is still included in too many places imho.

@michaelquellmalz
Copy link
Member

I agree that we could make the tests smaller in some cases (maybe using exhaustive only on selected platforms, especially not with quadruple precision).

Concerning the header files, there is still api.h, which basically just includes nfft3.h and infft.h, thus when someone looks at this, it seems that infft.h is part of the public api. Also I do not know what this file is good for and who uses this anyway. We could delete it, but that might break compatibility.

@jenskeiner jenskeiner merged commit 0564de9 into develop Oct 1, 2025
40 checks passed
@jenskeiner jenskeiner deleted the feature/macos-build branch October 1, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Test-related changes with no production code impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants