Add CountVectorizer

[ksew1]

Hi I added CountVectorizer as it would be useful in pipeline with Naive Bayes algorithms that we already implemented (for example in spam detection)

[josevalim]

Don't indent the docs by two spaces. They should be aligned with the closing """, as that's the one that controls indentation. :)

[ksew1]

Fixed 😄

[josevalim]

Suggested change

	## Options
	#{NimbleOptions.docs(@binarize_schema)}
	## Examples
	## Options
	#{NimbleOptions.docs(@binarize_schema)}
	## Examples

[ksew1]

Fixed 😄

[josevalim]

Under jit mode, you cannot convert a tensor to a number. You can try adding this test:

fun = &Scholar.FeatureExtraction.CountVectorizer.fit_transform(&1, indexed_tensor: true)
Nx.Defn.jit(fun).(Nx.tensor([[0, 1, 2], [1, 3, 4]])

The correct solution is to compute the default value for max_index inside defn.

[ksew1]

The problem is that it needs to be a number in order to create a tensor with this shape. Inside defn, I'm afraid it is not possible to dynamically obtain a number. We can make this a required option, so we do not need to use Nx.to_number().

[josevalim]

I see. So we should not use deftransform, because this certainly cannot be invoked inside a defn, as we don't yet support dynamic shapes. For now, it should be a regular def and maybe it should be called something else.

I believe this topic appeared in the past and we maybe discussed a possible contract for keeping code that doesn't work inside defn but I don't recall it right now. :) Maybe @msluszniak does?

[msluszniak]

Usually, if the shape was based on computations we either droped the options that force it (like percent of variation preserved in PCA) or develop some heuristics that roughly assesses the upper bound of the problematic shape, and then make computations on bigger tensor with some "padding".

[msluszniak]

Or make an option that is required via NimbleOptions

[josevalim]

Maybe an option is the way to go then and we could even have a helper function like CountVectorizer.size(n) that they could use to compute it.

[ksew1]

I made this required option and added helper function 😄

[josevalim]

The fit_transform function currently has an issue in that it accepts either tensors (which can be JITed) or a list of strings, which cannot. Generally speaking, our fit_transform functions can be jitted, so this would be breaking the tradition.

It seems passing a list of strings is implementing rather a simple tokenization algorithm? I think we should either move it elsewhere OR simply remove the functionality from here. My suggestion would be to remove it :)

[ksew1]

The fit_transform function currently has an issue in that it accepts either tensors (which can be JITed) or a list of strings, which cannot. Generally speaking, our fit_transform functions can be jitted, so this would be breaking the tradition.

It seems passing a list of strings is implementing rather a simple tokenization algorithm? I think we should either move it elsewhere OR simply remove the functionality from here. My suggestion would be to remove it :)

Removed this feature, now it operates only on tensor

[josevalim]

💚 💙 💜 💛 ❤️