Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix max fft size fiasco #281

Merged
merged 3 commits into from
Oct 12, 2024
Merged

fix max fft size fiasco #281

merged 3 commits into from
Oct 12, 2024

Conversation

weefuzzy
Copy link
Member

What is the road to where paved with?

This hopefully fixes #258

FFT had been altered some time ago to use a global setup object in order to cut down on these expensive things being redundantly created all over the place (quite possibly on the heap? Can't remember...). Instead an assumed hard limit of 2^16 Became A Thing. Unfortunately, nobody (i.e. me) told the rest of the codebase, which happily assumed that anything was possible. Moreover, (a) FFT::resize() is assumed by calling code not to fail (!?) and (b) its assert was useless because FFT was still cheerfully accepting fft sizes > 2^16, even if its setup object could never honour that.

(Seems like now would be a good time to have more unit tests)

So. I still like the idea of 'normal' FFT usage not spraying lots of these setup objects out into the world, but have attempted to accommodate the adventurous by allowing a local setup object in a std::optional when a chunkier size is requested.

So far, this is tested only to the extent of seeing if I could run NMF in Max with a FFT of 217. I will at least confirm that when fft size is <= 216, it continues to use the default path, but more dedicated tyre kicking by @tremblap will be helpful.

fix max fft size fiasco2
@weefuzzy weefuzzy added the bug Something isn't working label Oct 11, 2024
@weefuzzy weefuzzy self-assigned this Oct 11, 2024
@weefuzzy
Copy link
Member Author

The ubuntu CI is failing because one of the tests won't build. Seems like GCC has decided that in that test I'm trying to copy a move only object, but not at all clear why it's decided that now because this was always the case.

@tremblap
Copy link
Member

oh ubuntu is bailing at the tests (any idea why?), but I ran the SC unit tests and they all run fine... and nothing crashes now with the large fft size.

@tremblap
Copy link
Member

otherwise I should test on ubuntu and windows as this is potentially 'big' right?

@tremblap
Copy link
Member

I mean, running the UTs on locally compiled things?

@weefuzzy
Copy link
Member Author

Yes, whenever you're able!

@tremblap
Copy link
Member

doing it now as I practice this hard bass line I can't play to save my life :D

@tremblap
Copy link
Member

ok it refuses to build on Ubuntu 20. I get loads of similar errors to this:

/home/pa/Documents/source/flucoma-core/include/clients/rt/../common/../../algorithms/public/STFT.hpp:27:7: error: use of deleted function ‘fluid::algorithm::FFT& fluid::algorithm::FFT::operator=(fluid::algorithm::FFT&&)’
In file included from /home/pa/Documents/source/flucoma-core/include/clients/rt/../common/../common/FluidContext.hpp:17,

@weefuzzy
Copy link
Member Author

'it' is the tests, or a build for a cce? (and – what? – that function isn't deleted 😭 )

@tremblap
Copy link
Member

I've pulled the tip of main in SC, and the PR 281 in core, and building in ninja. I can send the full error stack but there are a lot.

@weefuzzy
Copy link
Member Author

e-mail it to me if you can. sounds horrible

@weefuzzy
Copy link
Member Author

Ok, received, thanks. That does give me a clue, now off to cppreference to see if I can figure out what exactly I've done to make gcc so sad (it's definitely this change, to do with adding a std::optional to the FFT class).

@tremblap
Copy link
Member

in case this helps explaining why MacOS doesn't bail but Ubuntu20 does: on the latter I have gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04.2)

Maybe this is why gcc is sad
@weefuzzy
Copy link
Member Author

Ok, think I figured it out. Can you check a full build again?

@tremblap
Copy link
Member

It builds in all 3 OSes, I am running the UTs in SC now in all 3

@tremblap
Copy link
Member

it all works perfect, you can merge

@weefuzzy weefuzzy merged commit 3ceddcd into flucoma:main Oct 12, 2024
3 checks passed
@weefuzzy weefuzzy deleted the max_fft branch October 12, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fluid.bufnmf~ crashes with fft size larger than 65k
2 participants