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 python3 examples for letor #26

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

Conversation

VaibhavKansagara
Copy link

No description provided.

@ojwb
Copy link
Contributor

ojwb commented Aug 6, 2019

Nothing actually references this new example so it won't appear anywhere in the documentation.

Also, generating the documentation runs the example code to check it actually works and produces the claimed results, so adding letor examples here would make letor (and so libsvm, etc) a requirement, which seems a bit undesirable. Letor is also a somewhat more advanced topic, so perhaps we should have a separate "volume" for letor which can be built separately.

code/python3/search_letor.py Outdated Show resolved Hide resolved
@VaibhavKansagara
Copy link
Author

Nothing actually references this new example so it won't appear anywhere in the documentation.

Also, generating the documentation runs the example code to check it actually works and produces the claimed results, so adding letor examples here would make letor (and so libsvm, etc) a requirement, which seems a bit undesirable. Letor is also a somewhat more advanced topic, so perhaps we should have a separate "volume" for letor which can be built separately.

I guess ayush_pandey has fixed all these issues in #18. This PR is supposed to be merged after #18 hence I don't think the above issues would be a problem.

@jaylett
Copy link
Contributor

jaylett commented Aug 8, 2019

Nothing actually references this new example so it won't appear anywhere in the documentation.

I guess ayush_pandey has fixed all these issues in #18. This PR is supposed to be merged after #18 hence I don't think the above issues would be a problem.

The "not appearing in" remains a problem, and when you do that you'll have to include expected output. The solution to depending on #18 is a bit fiddly — you can open a PR against @AyushP123's branch underlying #18 — so it's probably easiest to focus attention on getting #18 merged, then this can just merge to master.

@ojwb I actually wouldn't have a problem with a "full" build run requiring letor, since (if I remember correctly) inability to run the examples doesn't change the output, just causes build warnings that the output is different.

@ojwb
Copy link
Contributor

ojwb commented Aug 27, 2019

If the result of letor not being available is the same manual being generated plus some additional warnings, I guess that's OK (at least if the warnings are clear as to what's going on - if they mislead people to think there's a problem that will just generate support load).

But yes, we should get #18 tidied up and merged first. Unfortunately there are still a few outstanding issues with it.

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.

3 participants