Adapting armadillo's parser for mlpack(Removing Boost Dependencies)

[heisenbuug]

For background knowledge, look at these

• What do you think about boost dependencies
• Remove boost spirit dependency

Sample code to use the feature

#include <iostream>
#include <mlpack/core.hpp>

int main()
{
  arma::Mat<double> data;
  std::fstream file;
  
  file.open("data.csv");
  mlpack::data::load_data<double>(data, arma::csv_ascii, file);
  data.raw_print();
  
  return 0;  
}

[conradsnicta]

@heisenbuug

• Please don't copy the code from Armadillo without respecting the License requirements (Apache 2.0). You also need to acknowledge the author(s). It appears the code was taken from https://gitlab.com/conradsnicta/armadillo-code/-/blob/10.5.x/include/armadillo_bits/diskio_meat.hpp
• Don't use internal Armadillo functionality such as arma_cold, arma::cond_rel, arma::is_signed, arma::is_real, as that can be changed at any time. Armadillo's API policy states, in part:

Caveat: any function, class, constant or other code not explicitly described in the public API documentation is considered as part of the underlying internal implementation details, and may be removed or changed without notice. (In other words, don't use internal functionality).

[heisenbuug]

Hey, @conradsnicta Thank you for having a look.

• Please don't copy the code from Armadillo without respecting the License requirements (Apache 2.0). You also need to acknowledge the author(s). It appears the code was taken from https://gitlab.com/conradsnicta/armadillo-code/-/blob/10.5.x/include/armadillo_bits/diskio_meat.hpp

Sorry, I forgot to mention that file is taken from the armadillo, I will add that right away.

• Don't use internal Armadillo functionality such as arma_cold, arma::cond_rel, arma::is_signed, arma::is_real, as that can be changed at any time. Armadillo's API policy states, in part:
  Caveat: any function, class, constant or other code not explicitly described in the public API documentation is considered as part of the underlying internal implementation details, and may be removed or changed without notice. (In other words, don't use internal functionality).

@rcurtin @shrit how should we proceed then?
Maybe we can copy these to mlpack, i.e. even if removed from armadillo we can use them internally, not sure what to do.

[conradsnicta]

@heisenbuug I suggest writing the code from scratch, but using the Armadillo implementation as an inspiration and reference. You'll get a much better understanding of how things work.

The code in mlpack doesn't need arma_cold. Stuff like arma::cond_rel, arma::is_signed and arma::is_real is relatively simple to re-implement. You may be able to use existing C++11 functionality -- see the type_traits header: https://en.cppreference.com/w/cpp/header/type_traits

[shrit]

@heisenbuug I agree with @conradsnicta, it would be a better idea to rewrite the parser from scratch by using the same implementation of armadillo as a reference, also we need to mention that the implementation is inspired by Armadillo.

[heisenbuug]

@shrit @conradsnicta okay, I will start rewriting the parser from the scratch.

I have one doubt now that I am writing the code from the scratch we also need to follow
good design principles, right?

So, should I first think about how will I structure the whole parser? Or should I first just try
to reimplement the basic data loading code and we can think about the design along the way.

@conradsnicta, my initial idea was actually to implement a pandas like data structure for
C++, so now that I am implementing the parser itself, maybe I can implement some features
on the parser level?

Let me know what you all think.

[shrit]

I would vote for keeping the same implementation as the one provided by Armadillo because it would be impossible to write a better one during the 3 months of the summer. However, this does not mean copying the code directly from armadillo into mlpack, but this would require a complete rewrite of the code to provide the same performance and the same functionalities.

Once this step is complete it would be easier to add additional functionalities similar to one provided by pandas #2722.
The parser is an important part of mlpack core. We need to be sure that everything is working perfectly here.

Considering the design, the next release is mlpack 4.0, which means we can have some changes in the public API. However, we need to try to keep the same interface as the old one, with only minor changes if it is required most.

@heisenbuug Let me know if this is helpful 👍

[heisenbuug]

I would vote for keeping the same implementation as the one provided by Armadillo because it would be impossible to write a better one during the 3 months of the summer. However, this does not mean copying the code directly from armadillo into mlpack, but this would require a complete rewrite of the code to provide the same performance and the same functionalities.

Once this step is complete it would be easier to add additional functionalities similar to one provided by pandas #2722.
The parser is an important part of mlpack core. We need to be sure that everything is working perfectly here.

Considering the design, the next release is mlpack 4.0, which means we can have some changes in the public API. However, we need to try to keep the same interface as the old one, with only minor changes if it is required most.

@heisenbuug Let me know if this is helpful

So I will start by rewriting the basic data loading code, as suggested I will reimplement the functions that are not described in armadillo's public API documentation or I will try to find existing C++ 11 feature...

[shrit]

@heisenbuug, let me know if you need any help, do not hesitate in pushing the last modification, even if they are not complete yet, this will allow me to check and review it when I have time 👍

[heisenbuug]

@shrit We talked about how we want to modify Load functions so that user can choose whether he wants to use arma or coot

So we need to rewrite the function declarations of Load function which should be in this file, right? Also after the project completion, we won't have load.cpp, right?

Now based on whether the user selects namespace arma or coot we need to use different namespaces.

How can we achieve this behavior?
I came across this.

[shrit]

@shrit We talked about how we want to modify Load functions so that user can choose whether he wants to use arma or coot

So we need to rewrite the function declarations of Load function which should be in this file, right? Also after the project completion, we won't have load.cpp, right?

Yes, exactly, load.cpp should disappear when boost is removed.

Now based on whether the user selects namespace arma or coot we need to use different namespaces.

How can we achieve this behavior?
I came across [this](https://stackoverflow.com/questions/55612759/alternative-to-using-namespace-as-template-parameter

Yeah, in the meanwhile you can use MatType of arma::Mat<>
You can push the modification as soon as possible, it will be easier to add comments directly on the code.

[shrit]

@rcurtin @zoq Anything you have for this pull request or we can merge it?
I have reviewed it, nothing to add from my side.

[zoq]

@rcurtin @zoq Anything you have for this pull request or we can merge it? I have reviewed it, nothing to add from my side.

Will take another look later today.

[rcurtin]

It's all good from my side so @zoq if you're happy with it then feel free to go ahead and merge. :) Awesome work @heisenbuug!

[zoq]

Should we add an armadillo attribution for the code, something along the lines of this was inspired by armadillo's parser, or did the code deviate from the armadillo parse quite a bit?

[conradsnicta]

Should we add an armadillo attribution for the code, something along the lines of this was inspired by armadillo's parser, or did the code deviate from the armadillo parse quite a bit?

No attribution necessary -- the parser is pretty basic.

I suggest merging this in its current state, as it has been in development since May (ie. ~6 months). It's good enough. Further optional improvements can always be done afterwards.

[shrit]

@heisenbuug If you do not mind, I am going to commit all these propositions.

[shrit]

@conradsnicta Agreed, I am merging this one once these tests are passing.

[heisenbuug]

@heisenbuug If you do not mind, I am going to commit all these propositions.

Thank you for doing this. I was a bit busy with some other things and I was planning to look at them later. I hope all cases pass. If this gets merged soon I will open another PR for the parallel loading using OpenMP before the end of Sunday.

Can't wait to finally get this merged.