Updated dependencies and small bugfixes

giacbrd / ShallowLearn

An experiment about re-implementing supervised learning models based on shallow neural network approaches (e.g. fastText) with some additional exclusive features and nice API. Written in Python and fully compatible with Scikit-learn.

GNU Lesser General Public License v3.0

198 stars 30 forks source link

Updated dependencies and small bugfixes #18

Closed oroszgy closed 7 years ago

oroszgy commented 7 years ago

Hi, first of all thanks for your awesome work!

I have performed a few usability improvements on your library:

added a requirements.txt to make all the depencies explicit
updated fastext.py dependency to 0.8.3
updated Fastext default according to the original library
made the classifiers to return a meaningful prediction even when they would return None.

giacbrd commented 7 years ago

Hi, thanks for your contribution!

I have some remarks on this PR:

according to the CONTRIBUTING doc, you should sub-branch the develop branch, and not master (I mean, PR should be eventually merged to develop)
I don't understand why all those dependencies added in requirements.txt (eg. requests, boto, etc), furthermore for some dependencies I would not be stuck to a specific version (i.e. must use >= instead of ==)
where did you get those default values for fastText.py? (correct values should be these https://github.com/salestock/fastText.py#supervised-model no?)
I am also not sure about the modifications to the algorithm outputs, sorry. These days I have no time for analyzing them, probably next week

oroszgy commented 7 years ago

@giacbrd sorry for the bad PR. I'll close this one and will try it again as suggested.

giacbrd commented 7 years ago

Hi @oroszgy, don't worry. On the classifier output for undefined predictions: I don't know which is the best choice. Returning None allows the user to understand that he is incurring in bad predictions, but it is a value that can break code if it not properly managed. While returning the most frequent label hides this potential model problems to the user; moreover this makes the assumption that the estimated prior probability is the right choice for classifying "unpredictable" samples, I don't think this is always a desirable behaviour.

oroszgy commented 7 years ago

Indeed, it is a tricky question, how to handle undefined predictions, I was looking at this issue from the sklearn compatibility point of view, which definetly needs the replacement of None values.