Suggestion to switch to std::lgamma everywhere

[Shians]

So with some very crude profiling I see that most of the time in BASiCS_MCMC() is spent running R::lgammafn() for the log-likelihood. With some crude benchmarking I find that using std::lgamma can speed this up by 30+%.

I would suggest you switch the remaining R::gammafn() to std::lgamma from #include<cmath> and get some free performance.

[catavallejos]

Thanks @Shians for the suggestion. I will try this and let you know how it goes.

Best,

Catalina

[catavallejos]

Done!! Your suggestion works wonderfully!! Thank you so much @Shians !

[catavallejos]

Hi @Shians,

As I mentioned above, I tried your suggestion and we got a very good speed-up on our code (once again many thanks for that!). However, while everything works well on my computers (Mac OS), there is an error in the Bioconductor build report for linux and Windows:

error: 'lgamma' is not a member of 'std'

Have you encountered an issue like this before? As the new Bioconductor release is fast approaching, I will temporarily return to the old version (calling R::lgammafn()), but I would be great to sort this out.

Thanks!

Cata

[Shians]

Bit strange since you already use std::lgamma here. I'm guessing it's because std::lgamma is technically a C++11 and above feature, though some compilers implemented it ahead of standard, so you end up with it working some places but not others.

If this is the case all you need to do is add a C++11 requirement to your Makevars, so

CXX_STD = CXX11

Somewhere near the top of your src/Makevars and src/Makevars.win files. This will make it hard for people who haven't updated their C++ compilers in 10 years to use your software but I hope that's an acceptable loss.

[catavallejos]

Thanks for the quick reply! Indeed, it looks like the issue is the use of C++11. I will add that as a requirement for BASiCS (we shouldn't loose users, because our dependency to scran implies that C++11 is already a requirement for us anyway).

[catavallejos]

All fixed now. Thank you very much for your help!

FYI, in case you are interested, there is no need for updating the Makervars files. All what is needed is to add:

SystemRequirements: C++11
NeedsCompilation: yes

to the DESCRIPTION file.