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

Add new API-reference page, automated generation of .rst files, enable sphinx-gallery. Fix #1808 #1809

Closed
wants to merge 54 commits into from

Conversation

anotherbugmaster
Copy link
Contributor

@anotherbugmaster anotherbugmaster commented Dec 20, 2017

Fix #1808

Reformatted API reference, but there's a little bit ugly workaround there: reference (like [1]_) doesn't work correctly if there's another reference with the same number in the upper section. I explored sklearn repo, it seems like they're aware of it and trying not to do so or making the same workaround as me.

Autosummary sphinx module

Templating

@anotherbugmaster anotherbugmaster added documentation Current issue related to documentation difficulty medium Medium issue: required good gensim understanding & python skills labels Dec 20, 2017
@anotherbugmaster anotherbugmaster self-assigned this Dec 20, 2017
Copy link
Contributor

@menshikh-iv menshikh-iv left a comment

Choose a reason for hiding this comment

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

Really great change, big thanks @anotherbugmaste 💣

Of course, need to work more with structure now (missed some things), but you are on the right way!

@@ -32,6 +32,7 @@ help:

clean:
-rm -rf $(BUILDDIR)/*
-rm -rf generated/*
Copy link
Contributor

Choose a reason for hiding this comment

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

How related $(BUILDDIR) and generated/ ?

Copy link
Contributor Author

@anotherbugmaster anotherbugmaster Dec 22, 2017

Choose a reason for hiding this comment

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

In no way!

generated is the folder which contains new auto-generated .rst files. It ought to be purged every time documentation builds, otherwise autosummary will not update reference page.

@@ -0,0 +1,15 @@
:mod:`{{module}}`.{{objname}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a link in answer, how to "form" this templates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"""Get a list of the most important documents of a corpus using a variation of the TextRank algorithm [1]_.
Used as helper for summarize :func:`~gensim.summarization.summarizer.summarizer`
"""Get a list of the most important documents of a corpus using a
variation of the TextRank algorithm [1].
Copy link
Contributor

Choose a reason for hiding this comment

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

probably, you can use text identifiers for it, for example [some-paper]

Copy link
Contributor Author

@anotherbugmaster anotherbugmaster Dec 22, 2017

Choose a reason for hiding this comment

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

AFAIK this bug is somehow related to the trailing underscore in [reference_name]_. I'll try to find a better workaround.

Copy link
Contributor

Choose a reason for hiding this comment

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

@anotherbugmaster add links to comment for this PR about this bug, I'll try to find workaround too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@menshikh-iv
Copy link
Contributor

We'll continue work on it together with @anotherbugmaster in Jan

@anotherbugmaster
Copy link
Contributor Author

anotherbugmaster commented Dec 23, 2017

As for the bug message:

<autosummary>:1:Unknown target name: "1"

Here's what I've found:

https://groups.google.com/forum/#!topic/sphinx-users/JVNPh7AIKi8
Abjad/abjad#332

@menshikh-iv menshikh-iv added the incubator project PR is RaRe incubator project label Dec 26, 2017
@markroxor
Copy link
Contributor

@anotherbugmaster Just a generic gentle suggestion - Can you please change the title to something more informative? 😄

@anotherbugmaster anotherbugmaster changed the title Fix 1808 Reformat API reference Dec 27, 2017
@menshikh-iv menshikh-iv closed this Jan 8, 2018
@menshikh-iv menshikh-iv reopened this Jan 8, 2018
@menshikh-iv menshikh-iv closed this Jan 8, 2018
@menshikh-iv menshikh-iv reopened this Jan 8, 2018

html:
$(SPHINXBUILD) -b html $(ALLSPHINXOPTS) $(BUILDDIR)/html
rm -r $(BUILDDIR)/html/_sources
cp -r $(BUILDDIR)/html/* ../
cp -r $(BUILDDIR)/html/* .
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, I don't like the copying to the upper level though. Why are we doing it in the first place?

@@ -1,3 +1,3 @@
/*! jQuery Migrate v1.1.1 | (c) 2005, 2013 jQuery Foundation, Inc. and other contributors | jquery.org/license */
/*! jQuery Migrate v1.1.1 | (c) 2005, 2013 jQuery Foundation, Inc. and other contributors | jquery.org/license */
Copy link
Contributor

Choose a reason for hiding this comment

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

can you simply checkout this file, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@@ -344,6 +345,12 @@ def summarize_corpus(corpus, ratio=0.2):
list of str
Most important documents of given `corpus` sorted by the document score, highest first.

References
----------
.. [1] Federico Barrios, Federico L´opez, Luis Argerich, Rosita Wachenchauzer (2016).
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 added the RFM label Mar 5, 2018
@menshikh-iv menshikh-iv changed the title Reformat API reference Add new API-reference page, automated generation of .rst files, enable sphinx-gallery. Fix #1808 Mar 9, 2018
@piskvorky piskvorky mentioned this pull request Jun 25, 2018
@@ -301,10 +301,12 @@ def __init__(self, corpus=None, id2word=None, dictionary=None, wlocal=utils.iden
Only when `pivot` is not None will pivoted document length normalization be applied.
Otherwise, regular TfIdf is used.
slope : float, optional
Parameter required by pivoted document length normalization which determines the slope to which
the `old normalization` can be tilted. This parameter only works when pivot is defined.
It is the parameter required by pivoted document length normalization which determines the slope to which
Copy link
Owner

@piskvorky piskvorky Jun 27, 2018

Choose a reason for hiding this comment

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

This conflict is incorrectly resolved. Please continue with changes in #2102, which should be up-to-date with develop (plus with some additional fixes).

Copy link
Contributor

@menshikh-iv menshikh-iv Jun 28, 2018

Choose a reason for hiding this comment

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

@piskvorky author (@anotherbugmaster) can't commit to your branch (lack of permissions)

Copy link
Owner

@piskvorky piskvorky Jun 28, 2018

Choose a reason for hiding this comment

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

Why not add @anotherbugmaster as a committer? I cannot commit to his fork either (that's why I created another branch, directly in gensim, so we can all collaborate).

Copy link
Contributor

@menshikh-iv menshikh-iv Jun 28, 2018

Choose a reason for hiding this comment

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

@piskvorky done
@anotherbugmaster please fix #2102 branch (you have permissions for clone current repository (not your fork), commit & push directly to #2102 branch), sorry for bothering on vacation.

@menshikh-iv
Copy link
Contributor

Continue in #2102

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty medium Medium issue: required good gensim understanding & python skills documentation Current issue related to documentation incubator project PR is RaRe incubator project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants