CitrineInformatics / lolo

A random forest
Apache License 2.0
41 stars 12 forks source link

PLA-10433: Generic typing on Learner and TrainingResult interfaces #294

Closed sfriedowitz closed 2 years ago

sfriedowitz commented 2 years ago

The main changes are as follows:

One concern I have about these changes is how to handle the generics for classification. Internal to trees, we use Splitter[Char] and Learner[Char] because we encode all classification data as type Char. However, the higher level classes still extend Learner[Any], such as ClassificationTree <: Learner[Any] and RandomForestClassifier <: Learner[Any]. This works as far as everything compiles, but it feels a bit odd insofar as "classification" is being performed on different label types throughout the package.

TODOS:

bfolie commented 2 years ago

I'm not super familiar with the python release process. Do we need a version bump on that now, or only when we cut a Lolo release?

Bump the lolopy version as part of this PR. Then when you cut the release on GitHub, it sees that the version has been bumped and pushes to PyPi

sfriedowitz commented 2 years ago

@bfolie I rebased this into a feature branch I just created.

bfolie commented 2 years ago

Other than the one comment about changing TrianingData[I, L] to TrainingData[L], this LGTM