Now that I see the interface, I see a couple of problems that we have to discuss:

Right now the way we train a model e.g. RNN or LogisticRegression, etc. is we define all necessary model parameters (layer, regularization constant, etc.) and call the Train function. The Train takes the predictor and responses as input followed by an optional optimizer instance where we define the number of iterations/epochs, the threshold to terminate the optimization before we reach the maximum number of iterations, etc..

arma::mat predictor, responses;
Model model(...);

Optimizer optimizer(...);

model.Train(predictor, responses, optimizer);

The Evaluate function somehow replicates the functionality of the optimizer like the number of epochs. I'm not sure we should replicate the behavior since the way we train a model might be different from class to class. Like none of the existing classes can handle arma::field as input. There is no option to specify optimizer parameter, or what if we like to use the upcoming cross-validation feature as part of the evaluation pipeline. So I think instead of providing an evaluation function that takes a model and trains it we should see the Tasks as a generator that generates input/output samples for our model to train on. This enables us to train the model independently from the task. I guess besides the code replication another problem is I can't think of an easy solution to incorporate curriculum learning which might be necessary to get stable results into the class without providing a bunch of parameters for each task. So I propose the following:

CopyTask
{
 public:
  CopyTask(...) {}

  void Sample(arma::mat& predictor, arma::mat& responses)
  {
    // Generate predictor responses pair or pairs with the given parameter.
  }
}

CopyTask copyTask;
copyTask.Sample(predictor, responses);

model.Train(predictor, responses, optimizer);

Let me know what you think.

This sounds like a good idea to me, because I've already ran into similar issue when trying to implement SortTask - it was a huge copy-paste from CopyTask, which failed to work nonetheless 😢.
However, I would also propose to create one more header file with evaluation functions (sklearn-ish), like this:

#include <score.hpp>
auto score_fn = scorers::sequence_precision; // number of sequences model has got right.
// As an option:
// auto precision = scorers::bit_precision; 
// = number of dataset bits model has got right.

// ... CopyTask defined as in the previous comment

copyTask.Sample(predictor_train, responses_train);
copyTask.Sample(predictor_test, responses_test);
arma::mat responses_test_pred; // or whatever type is required;

model.Train(predictor_train,  responses_train, optimizer);
model.Predict(predictor_test,  responses_test_pred);
auto score = score_fn(responses_test, responses_test_pred);

IMHO, this will be even more in accord with the concern separation principle.

At the moment, I'm at an internship at our local Web-development company, though, so I hope to try out the ideas in this thread tonight or in the next few days.

It's just an idea but do you think since the score function depends on the task we should integrate the function into the Generator/Task class?

Maybe it is true, but I am not sure that the score function is the part of the problem. IMHO the task should specify only inputs and their respective outputs. The reason is that there are usually several viable ways to evaluate the distance between predictions and ground truths (e.g., in regression, you can use Euclidean / Manhattan metrics, use L1 / L2 regularizer with different degrees of regularization).

Even here, in the case of CopyTask, there are two plausible ways to evaluate the performance: bit precision and sequence precision.

That's true, there are often multiple ways to calculate the performance, and I think there are situations where you like to use the same functions during training, so I agree writing a new class/classes for the scoring functions is a good idea.

I would use const size_t maxLength and const size_t nRepeats here, we mostly use size_t instead of int if support of negative values isn't needed.

Minor style issue, please take a look at the Method Declarations sections in the style guide: https://github.com/mlpack/mlpack/wiki/DesignGuidelines#method-declarations.

Minor spelling issue; Numberinstead of Nomber.

Can you add a comment for the closing namespace here, to improve the readability?

Minor spelling issue: parameters instead of paramters.

Can you take a look at the Brace Placement section: https://github.com/mlpack/mlpack/wiki/DesignGuidelines#brace-placement. I wonder why the Style Check doesn't complain about this.

I think we can remove size and just use maxLength, what do you think?

Maybe we can use random value of size, so that we can feed variable-length data to our models?

Interesting idea, but I'm not sure we should do that inside the Generator class, since the required distribution depends on the user? If someone likes inputs with another length he could just call with another length right? Also, maybe it's a good idea, to use arma::mat instead of arma::field, since this way if someone likes to train only with a static length, he can directly use the output, since the model takes arma::mat as input.

I think we can avoid the extra copy here and use input(i) instead of `ìtem``, maybe I missed something?

Can you comment each parameter as we did for the other methods? Also, I think it would be good idea, to improve the general task description e.g. we could a simple example how to use the code and show an example output. What do you think?

Do you think it would be cleaner to rename the function to Generate?

Can you use the const qualifier here const maxLength and const nRepeats.

The data should be col major, also I was thinking that even if the data is binary, it might be good to use arma::col instead of `àrma::icolhere since the most of the models takearma::mat`` as input, and that way we could avoid a cast. What do you think?

Can we use const size_t batchSize here, to be consistent with the rest of the codebase.

Minor style issue, can you align the labels and batchSize parameter with the input parameter.

Perhaps batchSize isn't the best parameter name here? Do you think size could work?

I would like to leave the long name, because with size variable would be not as clear which size is meant (sample size? maximum sequence length? size of sequence elements?). The long name, in contrast, eliminates any chance to slip later.

Okay, sounds reasonable to me.

I don't think it's a good idea to set a seed here, what if someone likes to generate the same output e.g. to debug some problem? If someone needs different results in a single run he can set math::RandomSeed(std::time(NULL));before the run.

We could use RandInt from core/math/random.hpp here.

Do you think we can use repmat here, to simplify the code?

Looks like we can remove #include <cstdlib>and #include <ctime>.

Maybe I missed something, but it looks like we could avoid an extra copy here if we directly use labels(i) for the output of arma::conv_to<arma::colvec>::from(...

We should try to avoid auto: I think if people go through the code using auto does not support the general code understanding, since you have to look further to figure out the return values. Also here is a quote from the armadillo page:

Can I use the C++11 auto keyword with Armadillo objects and/or expressions?
Use of C++11 auto is not recommended with Armadillo objects and expressions. Armadillo has a template meta-programming framework which creates lots of short lived temporaries that are not handled by auto.

Can we work with trueOutputs instead of a copy? I guess the compiler would optimize that, but would be easy for us to do it.

Maybe you could simplify the expression here by doing something like: arma::accu(output.at(j) == prediction.at(j)) == prediction.n_elem, not sure if the expression necessary faster or slower.

I've looked through armadillo's docs and found an even better way: http://arma.sourceforge.net/docs.html#approx_equal

This method resolves all our problems in this place:

• It should be fast - it's armadillo native method (should be parallelized) + should stop after first mismatch (equality check, nothing more)
• No more code duplication - it accepts all armadillo objects, whether it's a matrix, vector or whatever else.

The only problem I see is, that approx_equalwas introduced in Version 6.700, but we support >= 4.200.0, so I guess what we could do here is to backport the function as we did for the e.g. ind2sub in core/arma_extend.

We could combine the functions for arma::field<arma::mat> and arma::field<arma::colvec> and could avoid some codeduplication by using a template:

template<typename eT>
double SequencePrecision(arma::field<eT> trueOutputs, arma::field<eT> predOutputs)
{
...
}

Please use camel casing for all names, for more information take a look at: https://github.com/mlpack/mlpack/wiki/DesignGuidelines#naming-conventions

Really neat code, I was wondering if we could use arma::sort_index to replace the key of std::pair.

I think we can remove #include <cstdlib>and #include <ctime> here.

Looks like there is a merge artifact, if you remove the duplication travis should build the PR again.

Not sure what happened here, but we should not remove the recently introduced cross entropy error function.

What do you think should we use linspace in combination with exp2 here?

I would go with arma::vec for the weights instead of std::vector feels more consistent with the rest of the codebase, what do you think?