Improve FastText documentation

[mpenkov]

• Updated fasttext module docstring to include better examples, design documentation
• Improved tutorial Jupyter notebook

[piskvorky]

Where is this model.epochs coming from? The model instantiation above shows no such variable.

[mpenkov]

Yes, the epochs parameter is optional, so not included during instantiation.

Unfortunately, there is also some confusion about its name: the FastText constructor uses iter to specify the number of epochs, whereas the superclass uses the proper name epochs.

The presence of the epochs parameter to the train function (which seems to override the one set in the constructor) also complicates matters.

[piskvorky]

Hm. If it's optional, let's not use it in train. Or if we use it in train, let's instantiate it explicitly. This neither-here-nor-there example is confusing ("where is this value come from?").

Regarding iter / epochs -- can you please rename it to epochs, consistently? I remember some discussion around this (cc @menshikh-iv @gojomo ), but can't imagine why we'd want both. At most we could support iter for a while as an alias, but with a clear deprecation warning.

This is a perfect opportunity to clean up some of the API mess, rather than piling on.

[mpenkov]

I agree regarding the cleanup. My preference would be to leave epochs/iter out of the constructor. The model doesn't need that parameter until training time.

[piskvorky]

Models in Gensim generally allow the trained_model = Constructor(params_including_training_params) pattern. So breaking that could be confusing to existing users (and a big change to backward incompatibility).

I'm not totally opposed though, especially if we still allow ctr params for a while with "deprecated" warnings. The API needs a clean up, and now is a good time.

Not a big priority though, and the documentation examples can already promote the instantiate then train, as 2 steps pattern.

[mpenkov]

It's not just training parameters you need to include in the constructor. It's also parameters for vocabulary creation. So you're managing at least 3 sets of separate parameters, 2 of which are duplicated by other methods of the class.

[piskvorky]

Yes, we should promote them as separate steps in docs. Question is, do we deprecate (certainly not remove) them from ctr?

[mpenkov]

I understand your motivation in not removing them (backward compatibility). Unfortunately, the current mess won't go away until we remove things like this.

I think the first step should be to deprecate them. After a while, we can remove them, perhaps in time for a major release.

If we want a one-liner way to instantiate and train, we can always write a pure function and promote that. That should make it easier for users to cut over to the cleaner API.

[piskvorky]

Yes, deprecation is what I suggest.

[piskvorky]

Why total_example=len(sentences)? Even if correct, it looks strange… would be my first question as a user.
Let's provide the answer here in the docs why total_examples is needed, what is its role.

[mpenkov]

It's correct. I agree that is confusing. The docstring for the train function attempts to clarify the situation.

Personally, I think if neither total_examples and total_words are specified, we should try to determine sensible defaults by looking at e.g. len(sentences). WDYT @menshikh-iv ?

[piskvorky]

Are you sure? I read the linked docs and still don't get why it's not len(new_sentences).

Please include some top-level intuition here. A short sentence on why this parameter is mandatory, and what should be its value, because it looks really strange and superfluous. +1 for sensible defaults.

[gojomo]

For train() to manage alpha correctly, and show meaningful progress estimates, it needs a good estimate of the size of the supplied-corpus - even when the corpus (as an iterable) may not self-report its length. In the typical case where the same corpus was just surveyed for its vocabulary, this value should be handy. (In the Word2Vec case, the count from the vocab-scan is cached inside the model for later consultation – unsure if the FT paths do this.) In the case where other/new data is being supplied, the caller should supply the right counts for the current data.

[mpenkov]

@gojomo If the corpus does self-report its length though, should we use that instead? If yes, which should we do:

1. total_examples = len(corpus)
2. total_words = len(corpus)

If the corpus does not self-report its length, then we could raise an exception with a heplful message. WDYT?

[gojomo]

If it's able to self-report its length, in count of texts, then yes, that would work as total_examples. But because streaming a corpus from an iterable is the motivating case for this interface, relying on that inside the method seems inappropriate to me. Leaving it the responsibility of the caller seems fine to me – and if they're lucky enough to have a corpus that reports its own length, they can supply it easily.

[mpenkov]

Also, while we're talking about simplifying the API, what do you think about removing the sentences and corpus_file parameters from the constructor? Currently, we have an inconsistency: in the constructor, we just pass sentences/corpus_file without total_examples and total_words parameters. In the train function, we include those additional parameters.

Instead of passing sentences in the constructor, the user can pass them in separately via the train function.

Pros:

• Simpler constructor. There will be less parameters.
• Easier to understand and consistent API.

Cons:

• Getting a trained model now takes two steps instead of one (instantiate, train). Not sure how much of a con this is given that the train function is more powerful than the constructor anyway - it has parameters that the constructor doesn't.

@menshikh-iv @piskvorky @gojomo What do you think?

The same thing also applies to the callbacks parameters.

[mpenkov]

But because streaming a corpus from an iterable is the motivating case for this interface, relying on that inside the method seems inappropriate to me.

@gojomo Why do you think it is inappropriate? We could do something like this:

if total_examples or total_words:
    pass  # nothing to do here
elif sentences and hasattr(sentences, '__len__'):  # could also check for callable, if necessary
    total_examples = len(sentences)
elif data_corpus and hasattr(data_corpus, '__len__'):
    total_examples = len(data_corpus)
else:
    raise ValueError(
        'unable to infer total_examples or total_words from the training source, '
        'please pass one of them explicitly'
    )

It looks ugly, but it allows to user to do something like:

model.train(sentences)

instead of

model.train(sentences, num_examples=len(sentences))

I feel the former is more Pythonic.

Finally, I think having two separate keyword parameters for the input is confusing for the user. In my opinion, it would look a lot simpler if we unified the two parameters, and dealt with untangling them in the implementation.

[piskvorky]

@mpenkov yes, I'd consider that "sensible" defaults. Thanks.

Agreed on unifying iter/epoch. How would that work? Keep both as acceptable input (internally unify to one), no deprecations? Deprecate one? Which one?

[mpenkov]

@piskvorky OK. I think it's worth dealing with API refactoring in a separate PR, for two reasons:

1. Such changes risk introducing regressions, and I'd rather not have them together with the relatively safe documentation changes.
2. We've already spent much effort refactoring FastText, and it may be prudent to wait for things to stabilize (e.g. fix introduced regressions) before proceeding.

To answer your question, I think it makes sense to deprecate iter from the constructor. It's a poor name for a parameter, for three reasons:

1. It masks the built-in iter function.
2. It isn't obvious.
3. It's inconsistent. We use epochs everywhere else outside of the constructor.

[menshikh-iv]

Good start overall, I note several issues with rendering
1.

:py:class:`Model`

should be

:class:`~gensim.models._fasttext_bin.Model`

please always use full path for any reference (class/method/function)
2.

:py:mod:`gensim.models.fasttext`

should be

:mod:`gensim.models.fasttext`

:py:class:`FastTextVocab`

should be

:class:`~gensim.models.FastTextVocab`

Also, you can always check how page rendered, just click "Details" near CircleCI and open "Artifacts" tab, this contains all rendered html, like https://circleci.com/gh/RaRe-Technologies/gensim/2108#artifacts/containers/0

[piskvorky]

@mpenkov looks good, thanks! Do the docstring examples now cover all the confusion we saw from users (mailing list, github)? Any important workflows left, undocumented / under-documented?

[mpenkov]

@mpenkov looks good, thanks! Do the docstring examples now cover all the confusion we saw from users (mailing list, github)? Any important workflows left, undocumented / under-documented?

It's hard for me to claim we've resolved all the confusion, because I haven't thoroughly read all the feedback from the users. Nevertheless, this PR addresses (or begins to address, owing to the ongoing discussions above) several key problems that we identified in the task description:

1. Unnecessary parameters in the constructor (ongoing, plan to continue unti we reach consensus).
2. IO problems (key, vec, etc).
3. Lack of examples for training continuation.
4. Lack of design documentation.

@menshikh-iv Have I missed anything?

[menshikh-iv]

@piskvorky LGTM, wdyt?

[piskvorky]

Does the model really need both? IIRC, it only needs one of those, no?

[mpenkov]

You're right, I updated the example.

[piskvorky]

Just split() (or we'll be left with newlines and all sorts of unicode "whitespace").

Also, I'd be careful with such examples, since people love to copy-paste (and this is really not a good way to tokenize -- even gensim.utils.tokenize is better).

[mpenkov]

OK, I've improved the example with gensim.utils.tokenize

[piskvorky]

MyIter(object) (we still support Python 2).

[mpenkov]

OK, done.

[piskvorky]

This looks great! I wish all PRs were this meticulous (and useful).

I have some things I'd want to add to the docs, but it's nitpicking at this point. Probably best if we merge and I'll go over it myself here and there. Nothing critical.

[menshikh-iv]

too long line (should be <120) + full_model=False, not partial=False @mpenkov

[menshikh-iv]

Awesome works, great @mpenkov 🔥