Templatize spreadinterp and more cleanups #567

mreineck · 2024-09-24T14:02:10Z

This is a massive change to spreadinterp.cpp, both reordering the functions in the file to avoid forward declarations, and templatizing everything in there.

I'd be happy to get this in as soon as possible, but I appreciate that this will be very hard to review...

mreineck · 2024-09-25T15:58:13Z

There are now no more doubly-compiled sources in the library (there are still doubly-compiled tests however). The current state is not pretty, since I had to make a few compromises to arrive at this point, but from now on things should improve again.
Still, before continuing, I'd like to have some feedback whether this has a chance to be accepted.

mreineck · 2024-09-25T18:45:38Z

Current test failures seem to be related to network problems on the CI servers ... they failed to download FFTW.

mreineck · 2024-09-26T07:53:34Z

OK, I'll stop myself here and wait for the review.

Don't get me wrong: there is still much to be done to really gather the benefits of this restructuring ... but all these subsequent changes will be more or less localized, and they will be much more fun than it was getting to the current state.

My hope would be that this can be merged before any other of the open PRs, because incorporating any change on master into this PR will be extremely painful. Of course I'll be happy to help with adjusting the conflicts I cause in the other PRs!

src/finufft_core.cpp

mreineck · 2024-10-18T17:04:12Z

A few answers to questions from the Google doc:

why pointer args sometimes became std::vector(T) &thing instead of T* thing ? Is this just to declare const?

No, the reason is to let C++ do as much memory management for us as possible. Whenever we have a vector, C++ will clean it up for us when it goes out of scope (or when its surrounding object is destroyed) -> no need to worry about free etc.

But we can't do this in all places. Sometimes we store pointers in the plan structure that have been provided by the caller. That means we must not do any memory management on them, and we cannot use vectors for them (unless we copy all the contents, which we don't want to do).

Is nullptr better than NULL ? (finufft_core.cpp)

Definitely yes, but the semantic differences are subtle. When writing modern C++ (since C++11), always use nullptr. I can dig out some references if anyone is interested in the details.

[Edit: intuitive example in https://en.cppreference.com/w/cpp/language/nullptr]

mreineck · 2024-10-18T18:50:03Z

I'll add a comment to finufft_core.cpp explaining the strange looking declarations with the different types.
They can be avoided by moving the templates into a header file instead of a cpp file. I usually do this, but it can be a burden on compile time, so I tried to be a bit more conservative here.

DiamonDinoia · 2024-10-18T18:50:56Z

Yes, that was kind of a bonus change, since I was touching all those lines anyway. I should probably have left that out and done it later.

It also fixes: #547

DiamonDinoia · 2024-10-18T18:54:33Z

OK, you are right: simple_interfaces.cpp looks more daunting now, but mostly because of the long type names: CPX -> std::complex<float/double>. I can introduce shortcuts for those ... not sure how much better that makes things, but I'll experiment a bit.

In my projects, I usually define a namespace types with something like:

namespace finufft{
namespace types{
using f32 = float;
using f64 = double;
using i32 = ...
using u32 = ... 
... 
}
}

and so on.

Then in .cpp files I just do using namespace finufft:types.

For public facing code I might define a shorthand for the namespace using finufft::types=FI so that I can do something like FI::i32. Something that still is intuitive and lets the reader understand the types.

For disclosure:
I have some hatred for keywords like single, double, int, long. My humble option is that they should have never existed.

src/finufft_core.cpp

lu1and10

Thanks, it looks great.
With a quick scan, I only find several sanity check things.

FINUFFT_PRECISION_DEPENDENT_SOURCES seems not used in cmake, the comments or cmake code with FINUFFT_PRECISION_DEPENDENT_SOURCES might be removed.
Since src/finufft.cpp is renamed to src/finufft_core.cpp, any code comments or documentation under docs/ referring to src/finufft.cpp may be changed accordingly; for the removed file utils_precindep.cpp, docs still mentions next235even in utils_precindep.cpp, now it's moved to utils.cpp
The guru interface source is now defined with template instantiation in src/simpleinterfaces.cpp, while in include/finufft_eitherprec.h we still mention it's in finufft.cpp. Though the filename simpleinterfaces.cpp may hint the users that the guru interface source may not be defined in simpleinterfaces.cpp.

ahbarnett · 2024-10-19T01:00:39Z

THanks, Libin. Which begs the question, why can't we rename finufft_core.cpp back to finufft.cpp (after all) ? The name finufft.cpp would match the code history better (it's an edit of the same file). But I can see that it might confuse the headers: finufft.h is the public header API, whilst finufft_core.{cpp,h} go together. Martin, any thoughts? If finufft_core is less liable to get confused with the finufft.h API, I'm happy to keep the new version (if we update docs and a bunch of stale code comments..)

ahbarnett

Thanks for this nice and big piece of work, Martin!

I must admit that de-macroizing and removing all the fwd-declarations looks a lot more maintainable (for the most part, apart from the new boilerplate to wrap the new class methods as standard functions, plus the , instances of everything - is this just so they are compiled into the lib?)
Overall compile time is longer (maybe factor of two?). But it is all going to be simpler in the end.

{Following a little out-of-date due to email chats:)
Since you are now becoming a major contributor, one piece of feedback would be to add code comments to explain the rationale for various decisions/changes. This is at the micro but also macro-level. We cannot read your mind, and it's easier if you err on the side of too many rather than too few explanations. You could add overall plan to docs/devnotes.rst too. But there is a big-picture plan implicit in your PR (eg: defs.h and FLT are now only for tests, and will go away soon) that we all need to have "on paper", to understand it, and figure out what happened post-hoc! It also helps you to delegate to us what needs doing.

My suggestions are optional and mostly just questions to understand stuff (I am no templating/class expert).

Thanks, Alex

makefile

fortran/finufftfort.cpp

include/finufft/defs.h

src/spreadinterp.cpp

mreineck · 2024-10-19T09:28:47Z

I don't know why the Windows test fails. Is it known to have intermittent problems?

DiamonDinoia · 2024-10-19T09:47:56Z

I don't know why the Windows test fails. Is it known to have intermittent problems?

Due to the number of test we run. Sometimes the docker env messes up and tests do fail for no apparent reason. In these cases, a re-run usually fixes it. If it does not then it is likely to have a problem in the code.

mreineck · 2024-10-19T09:56:36Z

This was an actual unit test failure, i.e. the error of a transform was higher than the tolerance. I don't think this is due to docker issues; it could be due to non-repeatable random numbers though

lu1and10 · 2024-10-19T09:57:13Z

I don't know why the Windows test fails. Is it known to have intermittent problems?

Due to the number of test we run. Sometimes the docker env messes up and tests do fail for no apparent reason. In these cases, a re-run usually fixes it. If it does not then it is likely to have a problem in the code.

I don't know why the Windows test fails. Is it known to have intermittent problems?

this seems relate to #516
@ahbarnett fixed with finufft*d_test.cpp, the many finufft*dmany_test.cpp: test may still fail occasionally, rerun may pass.

lu1and10 · 2024-10-19T09:59:21Z

This was an actual unit test failure, i.e. the error of a transform was higher than the tolerance. I don't think this is due to docker issues; it could be due to non-repeatable random numbers though

see #516
one solution may just increase the tolerance a little bit.

mreineck · 2024-10-19T13:48:53Z

Coming back to this:

Which begs the question, why can't we rename finufft_core.cpp back to
finufft.cpp (after all) ?

Yes, we can do that, and I think it's a great idea. There was a need for finufft_core.cpp at an intermediate stage, where I was moving stuff bit by bit from the twice-compiled finufft.cpp to finufft_core.cpp that was just compiled once.

Is it OK to do that in a follow-up patch (which I'm already preparing)?

ahbarnett · 2024-10-21T16:54:53Z

Yes please do a follow-up PR :) (There is no need to change the name finufft_core, I realised, because of the confusion about what itg .h would be called, not to clash with the API finufft.h).

Are you ready to ok this PR and bring in? @DiamonDinoia @lu1and10 Thanks! Alex

mreineck · 2024-10-21T17:31:40Z

PR is now open, but don't look at the diffs yet, they'll look horribly until the current PR has been merged :-)
I have renamed finufft_core.cpp to finufft.cpp in there, but reverting this is no problem; let me know!

lu1and10 · 2024-10-21T17:37:31Z

I think it's good to merge as the follow-up PR #581 will fix the remainings.

DiamonDinoia

Minor things and some questions. One comment (the ceil) can be ignored I put it there as a memo for me mostly

DiamonDinoia · 2024-10-21T19:00:52Z

src/finufft_core.cpp

    printf("[%s] new plan: FINUFFT version " FINUFFT_VER " .................\n",
           __func__);

-  p->fftPlan = std::make_unique<Finufft_FFT_plan<FLT>>(
+  p->fftPlan = std::make_unique<Finufft_FFT_plan<TF>>(


can this be std::optional ? So to have it stack allocated

That might work; I don't have much experience with optional, so I'll try tomorrow.

I propose to bring this in, and deal with such tweaks in the next PR.

src/finufft_core.cpp

src/simpleinterfaces.cpp

src/spreadinterp.cpp

ahbarnett · 2024-10-22T15:11:56Z

makefile

 # all lib dual-precision objs (note DUCC_OBJS empty if unused)
-OBJSD = $(OBJS) $(OBJSF) $(OBJS_PI) $(DUCC_OBJS)
+OBJS = $(SOBJS) contrib/legendre_rule_fast.o src/fft.o src/finufft_core.o src/simpleinterfaces.o fortran/finufftfort.o $(DUCC_OBJS)


ahbarnett

I would like to bring this in since the few details can be hashed out in the next PR.
Thanks! Alex

ahbarnett · 2024-10-22T16:09:00Z

@DiamonDinoia I am squashing this in, and your remaining tweaks we can disciss in the the next PR. Thanks! Alex

mreineck added 16 commits September 24, 2024 14:56

rearrange functions to avoid forward declarations

1cf0dbd

simplify simple interfaces code

7de00c8

templatize spreadinterp

7b4d8e6

templatize utils.cpp

dab8745

adjust makefile

f6fab70

fix inconsistent prototype

3b120a7

another attempt

0faa049

another attempt

d136d81

another attempt

efc3592

more templatizing

bbdaa70

more templatizing

9074322

fixes

aca3e5b

more templatizing

f6da37f

no more precision-dependent sources in library

22baa47

update makefile

47a284a

fix typo

654da01

get rid od utils_precindep

f83b7d6

mreineck marked this pull request as ready for review September 25, 2024 18:45

mreineck added 3 commits September 26, 2024 08:17

start migrating to std::vector

79b9080

NULL -> nullptr, more vectors

ff8da04

more OOP and some warning fixes

f411b15

ahbarnett mentioned this pull request Oct 8, 2024

Deal with platforms that don't define single-precision fftw #179

Closed

Merge remote-tracking branch 'origin/master' into simplify_sources

3d102df

mreineck mentioned this pull request Oct 15, 2024

[Draft] add single-argument kernel evaluator #541

Merged

DiamonDinoia reviewed Oct 18, 2024

View reviewed changes

src/finufft_core.cpp Outdated Show resolved Hide resolved

remove finufft.cpp; remove blanket use of namspace std

9374600

add explanation for explicit template instantiations

622855c

DiamonDinoia reviewed Oct 18, 2024

View reviewed changes

src/finufft_core.cpp Show resolved Hide resolved

DiamonDinoia reviewed Oct 18, 2024

View reviewed changes

src/finufft_core.cpp Outdated Show resolved Hide resolved

mreineck added 2 commits October 18, 2024 21:02

shorten simpleinterfaces

8238568

pass options by reference

ca14063

lu1and10 reviewed Oct 19, 2024

View reviewed changes

ahbarnett approved these changes Oct 19, 2024

View reviewed changes

mreineck added 2 commits October 19, 2024 10:04

address review comments

74ef403

revert BIGINT->UBIGINT change, seems to have bigger ramifications

60611eb

mreineck added a commit to mreineck/finufft that referenced this pull request Oct 19, 2024

merge changes from flatironinstitute#567

feca94a

mreineck mentioned this pull request Oct 21, 2024

Follow-up to the big reorg PR #581

Closed

lu1and10 approved these changes Oct 21, 2024

View reviewed changes

DiamonDinoia reviewed Oct 21, 2024

View reviewed changes

ahbarnett reviewed Oct 22, 2024

View reviewed changes

ahbarnett approved these changes Oct 22, 2024

View reviewed changes

ahbarnett merged commit 5340e0b into flatironinstitute:master Oct 22, 2024
167 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Templatize spreadinterp and more cleanups #567

Templatize spreadinterp and more cleanups #567

mreineck commented Sep 24, 2024 •

edited

Loading

mreineck commented Sep 25, 2024

mreineck commented Sep 25, 2024

mreineck commented Sep 26, 2024 •

edited

Loading

mreineck commented Oct 18, 2024 •

edited

Loading

mreineck commented Oct 18, 2024

DiamonDinoia commented Oct 18, 2024

DiamonDinoia commented Oct 18, 2024 •

edited

Loading

lu1and10 left a comment

ahbarnett commented Oct 19, 2024 via email •

edited

Loading

ahbarnett left a comment

mreineck commented Oct 19, 2024

DiamonDinoia commented Oct 19, 2024

mreineck commented Oct 19, 2024

lu1and10 commented Oct 19, 2024

lu1and10 commented Oct 19, 2024 •

edited

Loading

mreineck commented Oct 19, 2024

ahbarnett commented Oct 21, 2024

mreineck commented Oct 21, 2024

lu1and10 commented Oct 21, 2024

DiamonDinoia left a comment •

edited

Loading

DiamonDinoia Oct 21, 2024

mreineck Oct 21, 2024

ahbarnett Oct 22, 2024

ahbarnett Oct 22, 2024

ahbarnett left a comment

ahbarnett commented Oct 22, 2024

Templatize spreadinterp and more cleanups #567

Templatize spreadinterp and more cleanups #567

Conversation

mreineck commented Sep 24, 2024 • edited Loading

mreineck commented Sep 25, 2024

mreineck commented Sep 25, 2024

mreineck commented Sep 26, 2024 • edited Loading

mreineck commented Oct 18, 2024 • edited Loading

mreineck commented Oct 18, 2024

DiamonDinoia commented Oct 18, 2024

DiamonDinoia commented Oct 18, 2024 • edited Loading

lu1and10 left a comment

Choose a reason for hiding this comment

ahbarnett commented Oct 19, 2024 via email • edited Loading

ahbarnett left a comment

Choose a reason for hiding this comment

mreineck commented Oct 19, 2024

DiamonDinoia commented Oct 19, 2024

mreineck commented Oct 19, 2024

lu1and10 commented Oct 19, 2024

lu1and10 commented Oct 19, 2024 • edited Loading

mreineck commented Oct 19, 2024

ahbarnett commented Oct 21, 2024

mreineck commented Oct 21, 2024

lu1and10 commented Oct 21, 2024

DiamonDinoia left a comment • edited Loading

Choose a reason for hiding this comment

DiamonDinoia Oct 21, 2024

Choose a reason for hiding this comment

mreineck Oct 21, 2024

Choose a reason for hiding this comment

ahbarnett Oct 22, 2024

Choose a reason for hiding this comment

ahbarnett Oct 22, 2024

Choose a reason for hiding this comment

ahbarnett left a comment

Choose a reason for hiding this comment

ahbarnett commented Oct 22, 2024

mreineck commented Sep 24, 2024 •

edited

Loading

mreineck commented Sep 26, 2024 •

edited

Loading

mreineck commented Oct 18, 2024 •

edited

Loading

DiamonDinoia commented Oct 18, 2024 •

edited

Loading

ahbarnett commented Oct 19, 2024 via email •

edited

Loading

lu1and10 commented Oct 19, 2024 •

edited

Loading

DiamonDinoia left a comment •

edited

Loading