Coot S01E01

[shrit]

@rcurtin This PR is a draft and may not compile, I want to get your opinion on these modifications because most of the remaining modifications are repetitions over all layers and init functions.

If the structure is good, then I will go over the other layers, if not I am happy to apply better suggestions.

[rcurtin]

Looks good, I think this is the right direction to go. My main comment is that I think MakeAlias() needs an offset parameter, and then you can combine it with MakeTmp().

[rcurtin]

I think that we shouldn't actually include Bandicoot anywhere in the mlpack headers since it's not a strict dependency. I had imaged a user would use bandicoot+mlpack in their source like this:

#include <bandicoot>
#include <mlpack.hpp> // detects that bandicoot is available and sets MLPACK_HAS_COOT or similar

I'm open to other possibilities if you have other ideas. But basically this would just mean there needed to be a check somewhere in prereqs.hpp or similar that would set MLPACK_HAS_COOT accordingly, and then we would need to wrap any usage of anything Bandicoot-specific (e.g. any explicit naming of coot::) in a macro guard.

Getting the tests to automatically test against Bandicoot will be a little trickier and involve some CMake changes, but we don't have to do that in this PR for sure. 👍

[shrit]

For sure, I agree, less dependencies is better, you know me, I am not too fan of dependencies 👍
Actually, by doing this I was thinking, that once we have made mlpack entirely generic, there is no reason to include armadillo internally at all, we can remove it, and push it to the user side how to include it.

[shrit]

I probably add it there by error while testing,

[shrit]

Actually, this is very hard to do, so maybe in the very far future, it also worth considering

[rcurtin]

We do need a linear algebra library; I don't think we can remove Armadillo as a dependency. Especially since Bandicoot itself depends on Armadillo.

[mlpack-bot]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍

[shrit]

I have no idea how this went to stale 😄

[rcurtin]

@shrit is it ready to review? Just let me know and I can do that 👍

[shrit]

@rcurtin not yet, probably there is a need for one or two PR's and then I need to do some cleaning.

[shrit]

I will move the part related to MakeAlias implementation for armadillo to a new PR, otherwise, it would not make sense to add the offset in the middle of the function, because I want to have a default value for it.
Keeping the offset parameter in the middle of the function means that I have to refactor half of the library which is something to avoid when possible