How should the classifier behave against empty strings?

[ibnesayeed]

Currently the Bayes classifier allows passing empty string for all training, untraining, and classification. Also, the strings that have nothing, but stopwords behave the same way. This means, we are essentially messing up with training count while no real training is happening.

I think, we should check the length of word_hash and if it is zero then we should just skip the training and untraing methods. If the same is the case when classify method is called, then it should return nil as the score for each category should be Infinity for empty strings.

I found this out while I was working on #125.

I can make a PR if this sounds a sensible option to do.

[ibnesayeed]

Another weirdness could happen when untrain is called more often than train for a category. Some counts will be negative. category_has_trainings? in this case returns false, but actual counts are still negative and they are not frozen at zero. Additionally, the total_trainings can also go negative. The scores will me messed up in either case.

[Ch4s3]

Yeah, this has been sort of a longstanding issue. I'm not sure what the best way to handle empty string would be.

[ibnesayeed]

I have given my recommendation already in the second paragraph of the first post. Without this being resolved, writing tests for #129 would be tricky.

[Ch4s3]

Ahh sorry, I think I skimmed this one on my phone. Yeah, a length check is probably the right way to go. I wonder if we should log anything out if a user does that.

[ibnesayeed]

Logging is fine, but it should be an information level logging, not a warning or error. Because due to stopword filtering it might get empty unintentionally/unknowingly.

[Ch4s3]

It occurs to me that text.length == 0 fails for input like " ".

We need a blank?() method

[ibnesayeed]

It occurs to me that text.length == 0 fails for input like " "

In what context? In general Ruby code " ".length => 1 would be the case.

[Ch4s3]

That's what I mean. the string " " is empty, but has a length greater than 0.

Now, this is only an issue if we check for string emptiness before calling the Hasher to get the word hash, which probably makes sense in most cases you wouldn't want to execute any of that code on an empty string. This still leaves the issue of what to do about text that only has stopwords and the word_hash returns empty.

[ibnesayeed]

I would just check the length of word_hash which would be empty whether a blank string "" was called, something containing only non-letters was called such as " " or "...? " etc., or only stopwords were passed such as to be or not to be. In all the cases the returned word_hash` would be empty.

[Ch4s3]

Yeah, I guess I was just thinking of a case where you're iteratively training over a large csv or something and it hits a bunch of blank data and wastes time trying to hash that stuff. But your solution is a lot simpler, so let's do that. I may also add a simple return if text.length == 0 check to the train and untrain methods as well.

[ibnesayeed]

We can check the emptiness of the supplied string before calling the Hasher, to avoid unnecessary call to the stack of methods in Hasher, but this would be a micro-optimization, because in many practical applications, empty strings wont be passed, so checking for emptiness just to avoid Hasher call will perhaps cost more when accumulated for all the number of attempts that are not empty.

[Ch4s3]

Does #132 solve this in a way that facilitates testing on #125?

[ibnesayeed]

Also, making such decisions should be done in the done in Hasher (if at all), not in the main classifier methods like train, untrain, or classify. The rationale here is that if we allow support for custom tokenizer, then this early decision making will come into it's way. A custom tokenizer might want to preserve the leading and/or trailing spaces (so is the case with letter based n-grams), while emptiness check would call .strip! method that will change the essence of the token. So, it is best to leave it to the tokeniser to decide what a legitimate token is.

[ibnesayeed]

@Ch4s3: Does #132 solve this in a way that facilitates testing on #125?

Added some inline reviews and more comments in the PR.

[Ch4s3]

closed by #132