Bandicoot adaption #352

zoq · 2023-01-03T01:13:30Z

Initial Bandicoot adaption.

conradsnicta · 2023-01-05T15:15:59Z

.appveyor.yml

@@ -1,5 +1,5 @@
 environment:
-  ARMADILLO_DOWNLOAD: "https://sourceforge.net/projects/arma/files/armadillo-9.800.6.tar.xz"
+  ARMADILLO_DOWNLOAD: "http://ftp.fau.de/macports/distfiles/armadillo/armadillo-8.400.0.tar.xz"


@zoq is that version downgrade deliberate?
recent releases of ensmallen require armadillo 9.800+

rcurtin · 2023-01-05T15:17:56Z

include/ensmallen_bits/eve/eve.hpp

-  typename std::enable_if<IsArmaType<GradType>::value,
-      typename MatType::elem_type>::type
+  typename std::enable_if<IsArmaType<GradType>::value ||
+      coot::is_coot_type<GradType>::value, typename MatType::elem_type>::type


One alternate idea here would be to make a combined IsArmaOrCootType traits class and use that instead.

rcurtin · 2023-01-05T15:23:36Z

include/ensmallen_bits/ada_bound/ada_bound_update.hpp

          parent.iteration);
-      const ElemType biasCorrection2 = 1.0 - std::pow(parent.beta2,
+      const ElemType biasCorrection2 = 1.0 - pow(parent.beta2,


Any particular reason to un-qualify std::pow()? For arma:: functions like arma::sqrt(), it's clearly necessary so that ADL will work, but it doesn't seem necessary here. (I don't have a problem with it, I'm just curious if I overlooked something.)

rcurtin · 2023-01-05T15:24:57Z

include/ensmallen_bits/utility/randn.hpp

+
+template<typename ElemType>
+typename std::enable_if<coot::is_coot_type<ElemType>::value, ElemType>::type
+randn(const size_t rows, const size_t cols)


I think that the RNGs I just merged to bandicoot should match the Armadillo API, so I think you should be able to remove these functions and just call randn<MatType>(...) directly, etc.

rcurtin · 2023-01-05T15:26:07Z

This is super awesome! I'm still working through the related Bandicoot changes and opening PRs, but little by little I'll get everything opened in MRs there so that this will work. I only took a quick glance for now, but once I can properly test with Bandicoot I'll do a more detailed review. 🚀

shrit · 2024-01-30T12:14:24Z

@zoq I tried to review it, but there are massive amount of things that needs to be completed, this looks to me more of a draft at this stage, let me know if you need any help

zoq · 2024-01-30T14:07:13Z

I think all of the SGD based optimizers are working (let me know if you miss one), which I think is what you will need, to move forward with the mlpack part? Some of the optimizers will have to wait for downstream implementations of e.g. qr.

So not sure what you mean by "massive amount of things that needs to be completed"?

Signed-off-by: Omar Shrit <[email protected]>

rcurtin

Awesome to finally see this come together. I reviewed most of the optimizer implementations, but still have to look at the CMake configuration and the tests.

For the tests, I think that the only function really worth testing with Bandicoot is the LogisticRegressionTestFunction; almost all of the other ones compute the objective via individual element access, or operate on matrices so small that there is no hope of a GPU ever being fast enough (just the communication costs alone will be way too high).

Ideally I would hope that for at least this initial PR, we can get to a state where:

SGD and L-BFGS are reasonably fast enough that we can run the logistic regression tests. (I may need to do a little Bandicoot tuning and implementation behind-the-scenes.)
Other optimizers will at least compile with Bandicoot matrices, but we don't actually need to run the tests if they are slow. It should be straightforward enough to run with timing reports to see the optimizers where Bandicoot is obscenely slow (probably due to element access). In those cases, we can just add a test that creates the optimizer and either does 1 iteration only, or otherwise terminates immediately---then, the test is more of a check that that optimizer will at least compile with Bandicoot matrices.

I left some comments throughout, but often times the comment could be applied in many places but I only left it once. Let me know what you think, happy to help out with implementation or if there are other missing parts of Bandicoot that would make life easier. 😄

rcurtin · 2024-11-05T23:11:01Z

include/ensmallen_bits/ada_belief/ada_belief.hpp

-  typename std::enable_if<IsArmaType<GradType>::value,
-      typename MatType::elem_type>::type
+  typename std::enable_if<IsArmaType<GradType>::value ||
+      IsCootType<GradType>::value, typename MatType::elem_type>::type


We can probably make a combined traits class that is something like this:

template<typename MatType> struct IsMatType { constexpr static bool value = IsArmaType<MatType>::value || IsCootType<MatType>::value; };

Maybe that would help clean things up a little bit?

rcurtin · 2024-11-05T23:12:16Z

include/ensmallen_bits/cd/cd_impl.hpp

@@ -84,7 +85,8 @@ CD<DescentPolicyType>::Optimize(
      break;

    // Update the decision variable with the partial gradient.
-    iterate.col(featureIdx) -= stepSize * gradient.col(featureIdx);
+    /* iterate.col(featureIdx) -= stepSize * gradient.col(featureIdx); */
+    iterate.col(featureIdx) -= gradient.col(featureIdx);


Should we add stepSize back in?

rcurtin · 2024-11-05T23:13:11Z

include/ensmallen_bits/cd/descent_policies/random_descent.hpp

-    return arma::as_scalar(arma::randi<arma::uvec>(
-          1, arma::distr_param(0, function.NumFeatures() - 1)));
+    return randi<size_t>(
+        arma::distr_param(0, function.NumFeatures() - 1));


Making this generic for Bandicoot might require something like GetFillType (but adapted for distr_param) from mlpack in src/mlpack/core/util/using.hpp.

rcurtin · 2024-11-05T23:13:22Z

include/ensmallen_bits/cmaes/cmaes_impl.hpp

@@ -381,4 +381,4 @@ typename MatType::elem_type CMAES<SelectionPolicyType,

 } // namespace ens

-#endif
+#endif


Can we re-add the trailing newline? :)

rcurtin · 2024-11-06T00:13:52Z

include/ensmallen_bits/wn_grad/wn_grad_update.hpp

+      parent.b += pow(stepSize, 2.0) / parent.b *
+          pow(norm(gradient), 2);


Suggested change

parent.b += pow(stepSize, 2.0) / parent.b *

pow(norm(gradient), 2);

parent.b += pow(stepSize, 2.0) / parent.b * pow(norm(gradient), 2);

Just a little cleanup 😄

rcurtin · 2024-11-06T14:52:01Z

include/ensmallen_bits/smorms3/smorms3_update.hpp

-          { return std::min(v, (typename MatType::elem_type) stepSize); } );
-
-      iterate -= gradient % x / (arma::sqrt(g2) + parent.epsilon);
+      MatType x = min((g % g) / (g2 + parent.epsilon), lr);


Could you avoid lr by using clamp() here to ensure that no element is greater than stepSize?

rcurtin · 2024-11-06T14:52:45Z

include/ensmallen_bits/spalera_sgd/spalera_stepsize.hpp

-        if (arma::any(arma::vectorise(learningRates) <= 1e-15))
+        //if (any(vectorise(learningRates) <= 1e-15))
+        /* if (min(vectorise(learningRates)) <= 1e-15) */
+        if (learningRates.min() <= 1e-15)


I'm pretty sure this is a better implementation in Armadillo too.

rcurtin · 2024-11-06T14:53:16Z

include/ensmallen_bits/spsa/spsa_impl.hpp

@@ -43,17 +43,18 @@ typename MatType::elem_type SPSA::Optimize(ArbitraryFunctionType& function,
                                           MatType& iterate,
                                           CallbackTypes&&... callbacks)
 {
-  // Convenience typedefs.
+ // Convenience typedefs.


Suggested change

// Convenience typedefs.

// Convenience typedefs.

Oops, a space went missing.

rcurtin · 2024-11-06T14:53:53Z

include/ensmallen_bits/spsa/spsa_impl.hpp

@@ -94,8 +95,7 @@ typename MatType::elem_type SPSA::Optimize(ArbitraryFunctionType& function,
    const double ck = evaluationStepSize / std::pow(k + 1, gamma);

    // Choose stochastic directions.
-    spVector = arma::conv_to<arma::Mat<ElemType>>::from(
-        arma::randi(iterate.n_rows, iterate.n_cols,
+    spVector = conv_to<ProxyMatType>::from(randi(iterate.n_rows, iterate.n_cols,
        arma::distr_param(0, 1))) * 2 - 1;


I think the distr_param would need to be adapted here to be generic.

rcurtin · 2024-11-06T14:57:40Z

include/ensmallen_bits/utility/proxies.hpp

+randu(const size_t rows, const size_t cols)
+{
+  #ifdef USE_COOT
+  return coot::randu<ElemType>(rows, cols);


I think that for many of these proxy functions, we can use the using trick that we do in mlpack.

rcurtin · 2024-11-13T15:38:24Z

I took a look into what to do about the FindBandicoot.cmake file. I think the result I like this most is this one: https://stackoverflow.com/questions/6580856/how-to-distribute-findxxx-cmake

Basically the idea would be to put FindBandicoot.cmake (and related files) in the Bandicoot repository, try to install them to ${CMAKE_ROOT}/Modules/ when the library is installed, and then also it would be a good idea to add a note to the README indicating that FindBandicoot.cmake can be used directly by copying it to another project. (And I guess we should do the same with Findensmallen.cmake and Findmlpack.cmake eventually.)

zoq added 3 commits December 5, 2021 19:19

Intial bandicoot - refactoring.

e5e71f1

Push latest changes.

db90c7d

CMAES changes.

522b473

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Jan 3, 2023

zoq removed s: unlabeled s: unanswered labels Jan 3, 2023

conradsnicta reviewed Jan 5, 2023

View reviewed changes

rcurtin reviewed Jan 5, 2023

View reviewed changes

conradsnicta added the s: keep open label Jan 9, 2023

zoq mentioned this pull request Jan 27, 2024

Coot 2: Change arma::function to function #391

Closed

Marcus and others added 12 commits January 30, 2024 16:22

Use FindCUDAToolkit for cmake >= 3.17, to search for cuda libs.

7d63133

Merge branch 'master' into coot-init

c3591ba

Signed-off-by: Omar Shrit <[email protected]>

Merge branch 'master' into coot-init

b8e5fab

Look for nvrtc libs.

4f519b2

Update Yogi optimizer to make use of coot.

b1833ca

Update IQN optimizer to make use of coot.

aa61fc3

Update SGD optimizer to make use of coot.

d961528

Update Adam optimizer to make use of coot.

435adf9

Update test cases.

57fd7ad

Update DE test case.

5caa998

Update the is_coot check to also work if coot is not available.

06a046c

Make sure we don't fail in case we can't find the bandicoot config.

3014446

rcurtin reviewed Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bandicoot adaption #352

Bandicoot adaption #352

zoq commented Jan 3, 2023

conradsnicta Jan 5, 2023

rcurtin Jan 5, 2023

rcurtin Jan 5, 2023

rcurtin Jan 5, 2023

rcurtin commented Jan 5, 2023

shrit commented Jan 30, 2024

zoq commented Jan 30, 2024

rcurtin left a comment

rcurtin Nov 5, 2024

rcurtin Nov 5, 2024

rcurtin Nov 5, 2024

rcurtin Nov 5, 2024

rcurtin Nov 6, 2024

rcurtin Nov 6, 2024

rcurtin Nov 6, 2024

rcurtin Nov 6, 2024

rcurtin Nov 6, 2024

rcurtin Nov 6, 2024

rcurtin commented Nov 13, 2024

		parent.b += pow(stepSize, 2.0) / parent.b *
		pow(norm(gradient), 2);

Bandicoot adaption #352

Are you sure you want to change the base?

Bandicoot adaption #352

Conversation

zoq commented Jan 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcurtin commented Jan 5, 2023

shrit commented Jan 30, 2024

zoq commented Jan 30, 2024

rcurtin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcurtin commented Nov 13, 2024