-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix documentation for *2vec
models
#2087
Conversation
This reverts commit feb3c32.
…s, correct types, blank lines at end of docstrings
…into document-any2vec
…ome intuitive information taken from the papers but also references to usage examples for users that do not wish to understand the underlying theory.
@piskvorky |
But I see no |
gensim/models/keyedvectors.py
Outdated
+---------------------------+--------------+------------+-------------------------------------------------------------+ | ||
| capability | KeyedVectors | full model | note | | ||
+---------------------------+--------------+------------+-------------------------------------------------------------+ | ||
| continue training vectors | no | yes | You need the full model to train or update vectors. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not ✅ and ❌? Much more visual and lively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@piskvorky because the table breaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found workaround 4c0b827
gensim/models/fasttext_inner.pyx
Outdated
# coding: utf-8 | ||
|
||
"""Optimized cython functions for training :class:`~gensim.models.fasttext.FastText` model.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing blank line after docstring (here and elsewhere).
gensim/models/doc2vec_inner.pyx
Outdated
# coding: utf-8 | ||
# | ||
# Copyright (C) 2013 Radim Rehurek <me@radimrehurek.com> | ||
# Licensed under the GNU LGPL v2.1 - http://www.gnu.org/licenses/lgpl.html | ||
|
||
"""Optimized cython functions for training :class:`~gensim.models.doc2vec.Doc2Vec` model.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing blank line after docstring (here and elsewhere).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strong -1: goes against standard Python docstring logic (blank line to delimit unrelated blocks). Also, ugly / harder to read for no good reason (or is there a reason?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goes against standard Python docstring logic
any PEP for it? Also see https://www.python.org/dev/peps/pep-0257/#one-line-docstrings "There's no blank line either before or after the docstring."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a PEP for it, but it looks like common sense. Unrelated code blocks should not be squashed together -- neither visually nor logically nor functionally.
Pretty sure the "no blank line before or after" applies to method docstrings, where it actually makes sense. Not separating module imports from documentation looks absolutely horrible.
gensim/models/utils_any2vec.py
Outdated
@@ -3,7 +3,6 @@ | |||
# | |||
# Author: Shiva Manne <s.manne@rare-technologies.com> | |||
# Copyright (C) 2018 RaRe Technologies s.r.o. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1: why this change? There definitely should be a blank line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…rect doc building)
Fixes to formatting, language, hyperlinks. This PR includes fixes from another documentation PR: #1944.
Some parts I didn't understand, so I couldn't fix:
iter
andepoch
parameters?update_weights()
do?delete_temporary_training_data()
for? (especially compared to KeyedVectors)alpha
andmin_alpha
vsstart_alpha
andend_alpha