Skip to content

Bandicoot intergration - #425

Merged
rcurtin merged 68 commits into
mlpack:masterfrom
zoq:bandicoot-integration
Aug 31, 2025
Merged

Bandicoot intergration#425
rcurtin merged 68 commits into
mlpack:masterfrom
zoq:bandicoot-integration

Conversation

@zoq

@zoq zoq commented May 8, 2025

Copy link
Copy Markdown
Member

Integrating Bandicoot into ensmallen, so we can build against Armadillo and Bandicoot.

@zoq

zoq commented May 8, 2025

Copy link
Copy Markdown
Member Author

Adding the GPU Jenkins job next, before pushing further changes, so I can make sure, that each commit builds.

Comment thread include/ensmallen.hpp Outdated

#include <armadillo>

#ifdef USE_COOT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zoq Suggest changing USE_COOT to ENS_USE_COOT here and in CMakeLists.txt.

This is for consistency with all the other macros used by ensmallen, where the ENS_ prefix is used. The prefix helps avoid potential clashes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, will change it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we can actually leave whether or not Bandicoot is included up to the user---so, if they want Bandicoot, they should write in their program

#include <bandicoot>
#include <ensmallen.hpp>

and then in this file, we can detect whether or not Bandicoot is available by checking for a new enough version like this:

#if defined(COOT_VERSION_MAJOR) && \
    ((COOT_VERSION_MAJOR >= 2) || (COOT_VERSION_MAJOR == 2 && COOT_VERSION_MINOR >= 1))
  #define ENS_USE_COOT    
#endif

That is the idea we have been planning on for mlpack, at least. Let me know if I overlooked something.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled in f9521ad.

@zoq
zoq force-pushed the bandicoot-integration branch 3 times, most recently from d1ebe01 to eb2b0c8 Compare May 8, 2025 16:37
@zoq
zoq force-pushed the bandicoot-integration branch from eb2b0c8 to c3059c3 Compare May 8, 2025 16:43
zoq added 6 commits May 8, 2025 14:43
…d and datatype.

2. Update IQN optimizer to build against Armadillo and Bandicoot.
3. Update IQN test cases to test against Armadillo and Bandicoot backend.
4. Update logisitic regression function, to support Armadillo and Bandicoot.
1. AdaBelief
2. AdaBound
3. AMSBound
4. AdaGrad
5. Adam
6. Adamax
7. AMSGrad
8. Nadam
9. Nadamax
10. OptimisticAdam
11. Eve
12. FTML
13. LBFGS
14. Lookahead
15. Padam
16. QHAdam
17. SMORMS3
18. SpaleraSGD
19. SPSA
20. SWATS
21. WNGrad
22. Yogi
23. MomentumSGD
24. NesterovMomentum
25. QuasiHyperbolic
26. SGD
27. SGDR
28. SnapshotSGDR
29. CNE
30. CMAES
31. ActiveCMAES
32. PSO
33. LBestPSO
34. MOEAD
35. AugmentedLagrangian
1. AdaDelta
2. AdaSqrt
3. BigBatchSGD
4. CoordinateDescent (CD)
5. GradientDescent
6. GridSearch
7. NSGA2
8. ParallelSGD
1. DifferentialEvolution (DE)
2. RMSProp
3. SimulatedAnnealing (SA)
4. SARAH
5. SARAH+
6. SVRG
7. BarzilaiBorweinDecay
8. DemonAdam
9. DemonSGD

@rcurtin rcurtin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully I won't break Github with all of these comments 😬 😬

Anyway this is great. I left a lot of comments but hopefully most of them are trivial. I tried to identify places where it would be better to keep things on the CPU instead of potentially moving them to the GPU. I'm not certain I identified every place but I did my best. Many of the test files had some strange indentation so it made it hard to review that part---can you see maybe what happened there?

It looks like some of the Jenkins CI nodes (hinode or polar) are down so some of the CI builds don't run. If you want to move those to GHA instead of Jenkins that works too, up to you.

Comment thread CMake/COOT_FindCLBlast.cmake
Comment thread CMake/FindBandicoot.cmake Outdated
Comment thread CMake/FindBandicoot.cmake Outdated
Comment thread CMake/FindBandicoot.cmake Outdated
Comment thread CMake/FindBandicoot.cmake Outdated
Comment thread tests/cmaes_test.cpp Outdated
Comment thread tests/moead_test.cpp
Comment thread tests/nsga2_test.cpp Outdated
Comment thread tests/sa_test.cpp Outdated
Comment thread tests/spsa_test.cpp Outdated
@zoq
zoq force-pushed the bandicoot-integration branch from bd09952 to 8286511 Compare May 21, 2025 20:06
Comment thread include/ensmallen_bits/pso/pso.hpp Outdated
@rcurtin

rcurtin commented Aug 28, 2025

Copy link
Copy Markdown
Member

Nice, everything is handled here and I think this is ready to go once we get it to pass the builds. I noticed that the IQN_LogisticRegressionFunction - arma::fmat test is failing in CI---that one actually fails quite a lot. In #426 I adapted that test to allow five trials (the LogisticRegressionFunctionTest() function takes that as the last parameter). You can either import that change here, or we can ignore the test failure and it will get fixed when #426 is merged; either way is fine with me.

@rcurtin rcurtin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good to me! There are some failing tests but these will be fixed by the FP16 support PR.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second approval provided automatically after 24 hours. 👍

@rcurtin

rcurtin commented Aug 31, 2025

Copy link
Copy Markdown
Member

We'll need to get the FP16 PR merged to fix some of the tests before a release, but, really great to finally get this in!

@rcurtin
rcurtin merged commit b77cf51 into mlpack:master Aug 31, 2025
4 of 10 checks passed
@zoq zoq mentioned this pull request Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants