SupervisedClassifier::train take data and target

[Peter]

daughter of ticket:287

related to ticket:259

Constructors should not take target and data, but instead we have two train functions: one for MatrixLookup? and one for MatrixLookupWeighted?.

This implies that the make_classifier function takes no arguments and can be implemented as a copy constructor.

[Markus Ringnér]

In [1157] this has been mostly fixed.

Left-overs:

• make_classifier in KNN/NBC/NCC should be checked, now they are potentially too minimal with regards to copying values for class members. Peter, please fix NBC, I will check the other two.

• In KNN pointers to data and targets received in training are stored and used when predicting. Should KNN keep copies of training data and targets?

• Peter, you mentioned something about reintroducing some tests for EnsembleBuilder? that were removed while SVM and the others were at different development levels. Can this be done now? If so can you fix it.

[Peter]

Replying to markus:

In [1157] this has been mostly fixed.

Left-overs:

• make_classifier in KNN/NBC/NCC should be checked, now they are potentially too minimal with regards to copying values for class members. Peter, please fix NBC, I will check the other two.

Sure.

• In KNN pointers to data and targets received in training are stored and used when predicting. Should KNN keep copies of training data and targets?

The data is extremely cheap to copy - basically 3 pointers, but I think in principle that classifier should not copy these things.

• Peter, you mentioned something about reintroducing some tests for EnsembleBuilder? that were removed while SVM and the others were at different development levels. Can this be done now? If so can you fix it.

I will look into that.

[Peter]

(In [1161]) testing to build ensemble of svm refs #318

[Peter]

Replying to peter:

Replying to markus:

In [1157] this has been mostly fixed.

Left-overs:

• make_classifier in KNN/NBC/NCC should be checked, now they are potentially too minimal with regards to copying values for class members. Peter, please fix NBC, I will check the other two.

Sure.

NBC::make_classifier is fine.

[Markus Ringnér]

make_classifier fixed for NCC and KNN too in [1164].

I guess we can close ticket if we agree that:

In the description of this ticket it is written:

"This implies that the make_classifier function takes no arguments and can be implemented as a copy constructor"

I have not implemented make_classifier as a call to the copy constructors (only default versions exist) since I think make_classifier should produce a copy but an untrained copy. Agree?

[Peter]

Agree