search

pmbaumgartner / fasttext-lite

2 stars 1 forks source link

add FastTextMultiOutputClassifier #3

Open sandypreiss opened 1 year ago

sandypreiss commented 1 year ago

Closes https://github.com/pmbaumgartner/fasttext-lite/issues/1

pmbaumgartner commented 1 year ago

@sandypreiss check the mypy error. I think you might just want to create your own class instead of inheriting from FastTextClassifier, and just inherit from the same things that FastTextClassifier does.

sandypreiss commented 1 year ago

This has been a nice OOP lesson. I think an abstract base class is the best solution here. Lmk what you think.

sandypreiss commented 1 year ago

@pmbaumgartner thoughts on this iteration? I didn't implement your suggestion to inherit from sklearn separately yet. I wanted to resolve the mypy error while sticking with this design (mostly as an exercise to distinguish the issue causing the mypy error from the question of how best to align with sklearn's design).

No worries if you still prefer inheriting from sklearn separately. I just want to make sure your opinion hasn't changed now that this version is working.