fix massive bottleneck

[RaphaelS1]

Fixes a huge bottleneck in cpp functions for vectorised WeightDisc (includes Matdist and Arrdist)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (255a666) 88.71% compared to head (dcd1d72) 88.71%.

❗ Current head dcd1d72 differs from pull request most recent head ddef09a. Consider uploading reports for the commit ddef09a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #296      +/-   ##
==========================================
- Coverage   88.71%   88.71%   -0.01%     
==========================================
  Files         112      112              
  Lines        9122     9115       -7     
==========================================
- Hits         8093     8086       -7     
  Misses       1029     1029              

Files	Coverage Δ
R/SDistribution_Arrdist.R	84.50% <100.00%> (-0.11%)	⬇️
R/SDistribution_Matdist.R	90.19% <100.00%> (-0.29%)	⬇️
R/SDistribution_WeightedDiscrete.R	90.00% <100.00%> (-0.17%)	⬇️
src/Distributions.cpp	92.28% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jemus42]

Any idea why R CMD Check (macOS-latest (release)) is stuck?
Is it possible to restart this?

[jemus42]

The R CMD Check (macOS-latest (release)) job is still "expected", but... hanging?

[bblodfon]

C++ not my strong point, but since data is a NumericVector as well, shouln't it be data[j]?

[jemus42]

Checking with https://teuder.github.io/rcpp4everyone_en/080_vector.html#accessing-vector-elements that should be correct. I wonder what actually happens when using data(j, i) in this case 🤔

[bblodfon]

still it would be better to have it consistent, since both x and data are of the same type

[bblodfon]

Verification question: if we never have the statement data(j) == x[k] TRUE, then the mat(k, i) (pdf at a new time point not included in the original x) will be zero and that's what we want, right?

[RaphaelS1]

Yup

[bblodfon]

1. Would it make sense to speed this up by removing the third loop? You pretty much search for the index where the time/data points match every time you scan the vector data (again C++ not my strongest point, maybe with the break and scanning with for you are already doing pretty good)
2. There are other places in the code where we can substitute () with [] to make it more consistent - pushed a commit for this

[RaphaelS1]

How would you remove the third loop? Can you demonstrate with R code?

[bblodfon]

Was writing the previous response late and didn't think it through, so what I initially thought is not going to work. But I asked ChatGTP and got something similar: an optimization that reduces the time complexity from O(nr * nc * n) to O(nr + nc * n) by precomputing the indices of elements in the 'data' array using a hashmap (!):

#include <unordered_map>

// Assuming mat, pdf, data, and x are appropriately defined

// Create a hashmap to store the indices of elements in the 'data' array
std::unordered_map<int, int> dataIndices;
for (int j = 0; j < nr; ++j) {
    dataIndices[data[j]] = j;
}

// Use the hashmap to populate the 'mat' matrix efficiently
for (int i = 0; i < nc; ++i) {
    for (int k = 0; k < n; ++k) {
        auto dataIndexIterator = dataIndices.find(x[k]);
        if (dataIndexIterator != dataIndices.end()) {
            int j = dataIndexIterator->second;
            mat(k, i) = pdf(j, i);
        }
    }
}

[jemus42]

Heh, looks like your ChatGPT-foo is better than mine, I didn't get too far 😬
I thought that using an ordered set should be the way to go based on how indices are, well, ordered, so surely using an ordered data structure has some sort of benefit?
But I also didn't think this through entirely because I think I still haven't wrapped my head around this fully.

[bblodfon]

@RaphaelS1 so I think I understand what the C_Vec_WeightedDiscretePdf does => gets a pdf matrix and subsets it to the x times (points) instead of the data it has. It's just the names of x and data are not so informative in this context. Some points for discussion:

1. Should we consider some name changing? I think data is like a xnew?
2. These matrix tranposes maybe slow down things? Wouldn't be more sense if the code worked as C_Vec_WeightedDiscretePdf(x, data, mat))?

[RaphaelS1]

Yes maybe but not in this PR

[bblodfon]

I think (1) would make sense to do now, (2) might be trickier

[bblodfon]

for (1) => #297

[RaphaelS1]

The R CMD Check (macOS-latest (release)) job is still "expected", but... hanging?

Because I have outdated branch protection settings