Coot 13, Change arma::fill to internal_compact to avoid confusion with user declarations

[shrit]

No description provided.

[conradsnicta]

I strongly advise against directly using arma::fill::fill_class<>. This is an implementation level detail that can be changed in future versions of Armadillo.

Only the following symbols are publicly documented within Armadillo docs: arma::fill::none, arma::fill::zeros, arma::fill::ones, arma::fill::eye, arma::fill::randu, arma::fill::randn, arma::fill::value. How those symbols are implemented is deliberately not described (more details below).

Extract from the on API stability section of Armadillo documentation:

... Any functionality within Armadillo which is not explicitly described in the public API documentation is considered as internal implementation detail, and may be changed or removed without notice.

In other words, arma::fill::fill_class<> is off-limits. Please treat it as inaccessible internal Armadillo stuff.

I'm planning to eventually shift all internal functionality to a separate "internal" namepace. Stuff like arma::fill::fill_class<> could become arma::internal::fill::fill_class<> or arma::internal::fill_class<>. To maintain compatibility with (well behaved) user code, publicly accessible symbols like arma::fill::zeros would be within the arma namespace via using statements.

[rcurtin]

@conradsnicta this one is tricky. I agree and would prefer not to use internal Armadillo types, but here we want one name we can use that can be both arma::fill::zeros and coot::fill::zeros. This helps us avoid the situation where mlpack developers can't use fill::, which would feel like an awkward restriction. Ideally writing internal mlpack code feels as much like writing Armadillo as possible, except to keep it general we avoid the arma::. Without some namespace-agnostic solution for fill::, we can't have any uninitialized matrices, which is sometimes desired. (i.e. avoid filling the matrix with zeros, since the next call fills it.)

Under those desiderata, the only solution I could come up with is inheriting from the base class underlying both arma::fill::zeros and coot::fill::zeros. Do you have any alternative suggestions? I'm out today so I don't think I'll be able to play with it until Monday. Maybe there is some clever trick that C++ allows that we haven't thought of yet.

[rcurtin]

Maybe we can cleverly use decltype to avoid the direct name arma::fill::fill_class? We would still be depending on the fact that arma::fill::zeros is a type and not an enum.

[conradsnicta]

There should be a way to do this through a short template meta-program. I envisage that specifying the fill type while creating a matrix (which can be either an Armadillo or Bandicoot matrix) can be done along these lines:

MatType X(100,100, typename GetFillType<MatType>::fill_zeros);

where GetFillType has specialisations for arma::Mat<eT> and coot::Mat<eT>. Not sure if the typename keyword is still strictly required for C++14 or C++17, as I recall that there were language improvements in that area.

PS. I'm still not convinced that abstracting away the matrix type is the right way forward, though that discussion should probably be continued in the contributors-chat repo. All of this has the smell of overengineering.

[rcurtin]

🤦 oh, I was way overthinking it, yeah, a simple template metaprogramming approach like that would work great. It's a couple more characters, but that's not an issue.

PS. I'm still not convinced that abstracting away the matrix type is the right way forward, though that discussion should probably be continued in the contributors-chat repo. All of this has the smell of overengineering.

I see where you're coming from, but I also don't see another reasonable way to allow mlpack to work on matrix types that aren't supported directly by Armadillo.

[conradsnicta]

@shrit @rcurtin Followup to the earlier discussion in #3687 (comment).

Here's a possible implementation that avoids the direct use of internal functionality within Armadillo and Bandicoot. This requires C++17 due to use of inline variables (to my current understanding of template voodoo). Under C++14 this may cause linking errors. Example usage is listed after the proposed code.

Feel free to copy'n'paste or adapt this into mlpack under the BSD license, though please include me as one of the authors.

template<typename MatType>
struct GetFillMode
  {
  };

// specialisation for Armadillo matrices
template<typename eT>
struct GetFillMode< arma::Mat<eT> >
  {
  static constexpr decltype(arma::fill::none ) none  = arma::fill::none;
  static constexpr decltype(arma::fill::zeros) zeros = arma::fill::zeros;
  static constexpr decltype(arma::fill::ones ) ones  = arma::fill::ones;
  static constexpr decltype(arma::fill::eye  ) eye   = arma::fill::eye;
  static constexpr decltype(arma::fill::randu) randu = arma::fill::randu;
  static constexpr decltype(arma::fill::randn) randn = arma::fill::randn;
  };

// TODO: to increase ease of use, include specialisations for arma::Col<eT> and arma::Row<eT>

#if defined(MLPACK_HAS_COOT)

// specialisation for Bandicoot matrices
template<typename eT>
struct GetFillMode< coot::Mat<eT> >
  {
  static constexpr decltype(coot::fill::none ) none  = coot::fill::none;
  static constexpr decltype(coot::fill::zeros) zeros = coot::fill::zeros;
  static constexpr decltype(coot::fill::ones ) ones  = coot::fill::ones;
  static constexpr decltype(coot::fill::eye  ) eye   = coot::fill::eye;
  static constexpr decltype(coot::fill::randu) randu = coot::fill::randu;
  static constexpr decltype(coot::fill::randn) randn = coot::fill::randn;
  };

#endif

Example usage:

arma::mat A(10, 10, GetFillMode<arma::mat>::randu);

PS. I haven't yet looked into an abstraction for arma::fill::value(scalar), as that's likely to be more involved. As a possible workaround this pattern can be used instead:

MatType A(10, 10, GetFillMode<MatType>::none);
A.fill(scalar); 

[rcurtin]

Thanks! That will definitely work.

I wondered about the code you wrote---instead of naming the type of arma::fill::zeros, it just uses decltype() to avoid doing that. One could actually do the same thing for the original strategy:

    namespace fill {
      // used when both Armadillo and Bandicoot are available
      class fill_zeros : public decltype(arma::fill::zeros), public decltype(coot::fill::zeros) { };

      static constexpr fill_zeros zeros;
    };

How do you feel about that implementation---is that using Armadillo internals in an undesired way?

I'm ambivalent about which to use, personally, I suspect the template metaprogramming selector approach is nicer to work with than the multiple inheritance approach.

PS. I haven't yet looked into an abstraction for arma::fill::value(scalar), as that's likely to be more involved. As a possible workaround this pattern can be used instead:

Yeah, I see, it is a little trickier but I think it could be done. Basically the correct function would need to be forwarded based on the given matrix type. I think that Bandicoot doesn't current implement fill::value though, and we also don't use it internally, so not a huge issue to get that right now.

[conradsnicta]

This doesn't directly touch Armadillo internals, so on the surface this looks fine. I think that when Armadillo internals are updated, mlpack-based code would only need a recompilation. Since mlpack is now header only, and almost all of Armadillo is in headers, there shouldn't be any breakage.

I recommend double checking that the multiple inheritance approach actually works. In other words, check that fill::none, fill::zeros, fill::ones, fill::randu etc still fill the matrices with correct values.

PS. To be on the safe side, I also recommend checking whether this requires C++17 or can work with C++14 without linking problems.

[shrit]

@rcurtin maybe it is the time for us to jump to C++17 to make this compile on the CI, because it is passing with no issue on my end

1. Currently we are close to May, which is the middle of the year.
2. We will need sometime to adjust and remove all the backporting that we have added

[shrit]

As discussed internally, I will get back to this after we move to C++17