search

BallisticLA / RandLAPACK

A high-performance C++ library for randomized numerical linear algebra
60 stars 6 forks source link

Benchmark RNG templating #64

Open TeachRaccooon opened 6 months ago

TeachRaccooon commented 6 months ago

Most benchmark scripts include a function definition like

template <typename T, typename RNG>
static void 
test_speed(int64_t m, int64_t n, int64_t runs, RandBLAS::RNGState<RNG> const_state)

which we invoke with lines like

test_speed<double, r123::Philox4x32>(std::pow(2, 10), std::pow(2, 5),  10, state);

Specifying r123::Philox4x32 as a template parameter shouldn't actually be needed. The compiler should be able to deduce it from the type of state. If we run into issues with compilers automatically deducing types then Riley would prefer us change the function definition to have a default type, along the following lines:

template <typename T, typename RNG = r123::Philox4x32>
static void 
test_speed(int64_t m, int64_t n, int64_t runs, RandBLAS::RNGState<RNG> const_state)
TeachRaccooon commented 5 months ago

@rileyjmurray so you would like me to remove the RNG template parameter from all functions that we explicitly pass the 'state' variable to, correct? And then the function signature, we would have the 'state' parameter defined as follows: RandBLAS::RNGState<> &state, with no specification of template parameter.

Another question is whether we want to restructure the way the algorithm object declaration/algorithm function calls are set up. At the moment, we would declare an object as such: RandLAPACK::RBKI<double, r123::Philox4x32> RBKI(false, time_subroutines, tol); having to specify a RNG template parameter, while the algorithm call function itself requires a state variable: RBKI.call(m, n, all_data.A.data(), m, b_sz, all_data.U.data(), all_data.VT.data(), all_data.Sigma.data(), state); Would it make more sense to specify the state upon object creation? or was there a reason why we decided against that approach?

rileyjmurray commented 5 months ago

That's probably what I'd suggest. Can you link to a few qualitatively different places in the code where you'd make this change? That'd help me be sure.

From: Max Melnichenko @.> Sent: Saturday, April 13, 2024 10:39 AM To: BallisticLA/RandLAPACK @.> Cc: Murray, Riley John @.>; Mention @.> Subject: [EXTERNAL] Re: [BallisticLA/RandLAPACK] Benchmark RNG templating (Issue #64)

@rileyjmurrayhttps://github.com/rileyjmurray so you would like me to remove the RNG template parameter from all functions that we explicitly pass the 'state' variable to, correct?

— Reply to this email directly, view it on GitHubhttps://github.com/BallisticLA/RandLAPACK/issues/64#issuecomment-2053665189, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACRLIFHP3SCIKX72ACNVO3TY5E7QVAVCNFSM6AAAAABFSAMIACVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJTGY3DKMJYHE. You are receiving this because you were mentioned.Message ID: @.***>

TeachRaccooon commented 5 months ago

@rileyjmurray

  1. The calls to <RandLAPACK::gen::mat_gen()> that repeatedly occur in both testing and benchmarking. Example: https://github.com/BallisticLA/RandLAPACK/blob/5432731e829a6e01ae939a1e3f09baddfa8dd1e2/benchmark/bench_RBKI/RBKI_runtime_breakdown.cc#L151
  2. Functions that invoke all algorithms in tests and benchmarks. Example: https://github.com/BallisticLA/RandLAPACK/blob/5432731e829a6e01ae939a1e3f09baddfa8dd1e2/benchmark/bench_RBKI/RBKI_runtime_breakdown.cc#L166C73-L166C145
  3. 'call' functions that belong to algorithm objects. However, my second suggestion would resolve this in a different way. Example: https://github.com/BallisticLA/RandLAPACK/blob/5432731e829a6e01ae939a1e3f09baddfa8dd1e2/RandLAPACK/comps/rl_rs.hh#L122
rileyjmurray commented 5 months ago

Okay, I think I jumped the gun in saying "yes" to

Would it make more sense to specify the state upon object creation? or was there a reason why we decided against that approach?

There was a reason why we decided against that. The biggest reason is that passing random states needs to be explicit whenever it's needed. We wouldn't want people to call an algorithm multiple times expecting that its random state is automatically being updated behind the scenes.

I think we should try to resolve this GitHub Issue over the course of a few PRs.

As for your first type of example, there's actually no need to specify any template parameters in a function call like https://github.com/BallisticLA/RandLAPACK/blob/5432731e829a6e01ae939a1e3f09baddfa8dd1e2/benchmark/bench_RBKI/RBKI_runtime_breakdown.cc#L151 The compiler will figure out the numerical precision by looking at all_data.A.data() and it'll figure out the RNG by looking at state. So I suggest you find the places in RandLAPACK that fall into this category and then open a PR that removes the unnecessary template arguments.

We have have separate PRs that address your second and third types of examples. (We'll have some discussion about how to best address those once you're ready to work on them.)

That sound good?

TeachRaccooon commented 5 months ago

@rileyjmurray I was thinking of taking care of the second type of issue in the currently-opened PR that updates the RandBLAS submodule for RandLAPACK. But it can be a separate PR if you think that should be the case.

The first type of issue is related specifically to matrix generators, we can discuss that separately.

rileyjmurray commented 5 months ago

@TeachRaccooon no I think that's a good idea. Go ahead and take care of it in the open PR.

TeachRaccooon commented 5 months ago

@rileyjmurray I addressed points 1 and 2 together with your comment about that in some cases, we do not need to specify any template parameters in PR #86. Lmk if anything looks off.