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

Update docsprint to reflect the new xapian-letor. #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AyushP123
Copy link

@AyushP123 AyushP123 commented Aug 10, 2017

I have updated the docsprint to account for changes in xapian-letor.

@jaylett
Copy link
Contributor

jaylett commented Aug 12, 2017

As I mentioned on IRC, this cannot maintain the structure of the documentation file from xapian-letor. I believe it needs to have a very brief introduction into what letor actually does (it reorders a match set to bring more relevant documents to the top, using a more sophisticated approach to calculating relevance than probabilistic weighting schemes like BM25), then show how to use it. Finally, at the end, it can talk about the background in more detail; but you don't have to understand it in order to use letor.

You should look at the rest of the repo to see how examples are done. They should be complete programs, out of which specific line ranges are included in the documentation. You'll have to write the examples in C++ (because language bindings aren't available for letor yet). The way this is done will also provide syntax highlighting for the code snippets.

And you must build documentation and look at it before opening a pull request. This PR has a number of RST syntax errors and incorrect uses which are obvious as soon as you look at the rendered output. This is the documentation equivalent of tidying up C++ source code: it must build without errors, and it should follow the conventions already established in the rest of the documentation, as far as possible.

A final note: all the stuff about extendability should be removed. As it stands, it's either obvious (to add a new Scorer, you subclass Scorer) or incorrect (users of Xapian don't have to put new scorers into the Xapian build tree, and indeed should not). Once we have basic documentation in place we can perhaps think of a way of explaining with a practical example how to extend letor.

I suggest you make the relevant changes then force push a new commit, and then add a comment here to indicate that it's ready for review; right now there are enough issues you can pick up yourself from the above without it being worth pointing every single one out, and given the need to restructure the document, I think fixup commits are going to be hard to follow.

@AyushP123 AyushP123 force-pushed the master branch 3 times, most recently from a7a1dd2 to 28aaf3a Compare August 21, 2017 05:30
advanced/learning_to_rank.rst Outdated Show resolved Hide resolved
advanced/learning_to_rank.rst Outdated Show resolved Hide resolved
advanced/learning_to_rank.rst Outdated Show resolved Hide resolved
advanced/learning_to_rank.rst Outdated Show resolved Hide resolved
advanced/learning_to_rank.rst Outdated Show resolved Hide resolved
advanced/learning_to_rank.rst Outdated Show resolved Hide resolved
conf.py Show resolved Hide resolved
conf.py Show resolved Hide resolved
conf.py Show resolved Hide resolved
conf.py Outdated Show resolved Hide resolved
code/c++/search_letor.cc Outdated Show resolved Hide resolved
code/c++/search_letor.cc Outdated Show resolved Hide resolved
code/c++/search_letor.cc Outdated Show resolved Hide resolved
@AyushP123
Copy link
Author

I have published the c++ documentation online here. Learning to Rank is published here. We can use this site to provide c++ documentation along with the default python documentation that we have.

Copy link
Contributor

@ojwb ojwb left a comment

Choose a reason for hiding this comment

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

Thanks for the update - a few minor things:

code/c++/search_letor.cc Outdated Show resolved Hide resolved
conf.py Outdated Show resolved Hide resolved
conf.py Outdated Show resolved Hide resolved
@AyushP123
Copy link
Author

Hi,
I am very sorry for the late fixup. There are more issues present in the documentation which need to be ironed out. I remember addy getting confused on a lot of topics. Will go through them again and create a new PR.

@VaibhavKansagara
Copy link

VaibhavKansagara commented Mar 8, 2019

@AyushP123 let me know if I can help with any of those issues. Also what do you think about adding documentation of xapian-letor-update in the main repository?.

@ojwb
Copy link
Contributor

ojwb commented Mar 11, 2019

Will go through them again and create a new PR.

I'd suggest it's probably worth addressing the last few issues here first so we can merge this first as it's already a significant improvement over the current situation.

Then a new PR can build on that.

@@ -29,7 +29,7 @@ using namespace std;

static void show_usage()
{
cout << "Usage: search_letor --db=DBPATH MODEL_METADATA_KEY QUERY\n";
cout << "Usage: search_letor MODEL_METADATA_KEY QUERY\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is wrong - it's only the --db= part which isn't expected - the DBPATH part is.

you have 'title' information in the collection with some xml/html tag or so
then add::

indexer.index(title,1,"S");
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting spaces after the commas in code would better match the other code examples.

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