Skip to content
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

Merged
merged 73 commits into from
Jun 20, 2018
Merged

Fix documentation for *2vec models #2087

merged 73 commits into from
Jun 20, 2018

Conversation

piskvorky
Copy link
Owner

@piskvorky piskvorky commented Jun 9, 2018

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:

  • what is the difference between the iter and epoch parameters?
  • what does update_weights() do?
  • what is delete_temporary_training_data() for? (especially compared to KeyedVectors)
  • relationship between alpha and min_alpha vs start_alpha and end_alpha

steremma and others added 30 commits February 28, 2018 16:11
…s, correct types, blank lines at end of docstrings
…ome intuitive information taken from the papers but also references to usage examples for users that do not wish to understand the underlying theory.
@menshikh-iv
Copy link
Contributor

menshikh-iv commented Jun 17, 2018

@piskvorky iter deprecated, epochs should be used always. We keep both parameters only by backward-compatibility reason.
About "optional": confusing situation - both optional, but one of it should be provided.

@piskvorky
Copy link
Owner Author

But I see no epochs in Word2Vec constructor, for example. How can iter be deprecated? I still don't get it.

+---------------------------+--------------+------------+-------------------------------------------------------------+
| capability | KeyedVectors | full model | note |
+---------------------------+--------------+------------+-------------------------------------------------------------+
| continue training vectors | no | yes | You need the full model to train or update vectors. |
Copy link
Owner Author

@piskvorky piskvorky Jun 17, 2018

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.

Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found workaround 4c0b827

# coding: utf-8

"""Optimized cython functions for training :class:`~gensim.models.fasttext.FastText` model."""
Copy link
Owner Author

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).

# 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."""
Copy link
Owner Author

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner Author

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?).

Copy link
Contributor

@menshikh-iv menshikh-iv Jun 18, 2018

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."

Copy link
Owner Author

@piskvorky piskvorky Jun 18, 2018

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.

@@ -3,7 +3,6 @@
#
# Author: Shiva Manne <s.manne@rare-technologies.com>
# Copyright (C) 2018 RaRe Technologies s.r.o.

Copy link
Owner Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@menshikh-iv menshikh-iv changed the title Fixes to *2vec docs Fix documentation for *2vec models Jun 20, 2018
@menshikh-iv menshikh-iv merged commit 90c22fd into develop Jun 20, 2018
@menshikh-iv menshikh-iv deleted the word2vec_docs branch June 20, 2018 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants