mljs / random-forest

Random forest for classification and regression.
https://mljs.github.io/random-forest/
MIT License
61 stars 21 forks source link

TODO before release #4

Closed targos closed 7 years ago

targos commented 7 years ago
jajoe commented 7 years ago

I'm not sure than the separation into two classes is a good idea. When we will have to add features (like subsampling or pruning for example) it could be easier to have the same base. If there are two classes, we will have to modify both classes to add features.

targos commented 7 years ago

This way of programming (having a base class to implement common code) is very Java-ish. In JS, we have the liberty to implement methods in functions that are not directly defined in the class. One of the advantages of this approach is that you can use separate files to write the different methods. It's true that we have to modify both classes, but it would be just to add 3 or 4 lines to call the common method.

JeffersonH44 commented 7 years ago

but it's a little bit weird to pass always the classifier to each method that is common to both classes (if we only use functions), I mean, it looks better with a base class, no? @targos

targos commented 7 years ago

It's a refactoring I would like to do myself but it's not needed for the release. I removed it from the list.

maasencioh commented 7 years ago

Even I think that we could check a little bit more the API, the code was published in bc09bab3f2f252a61efd33885649a14fd96f7098 and the list was fulfilled, so I'm closing this