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 io.anserini.ltr.FeatureExtractorCli for argument -collection clueweb #689

Merged
merged 5 commits into from
Sep 18, 2019

Conversation

mam10eks
Copy link
Contributor

@mam10eks mam10eks commented Jun 8, 2019

This pull request will close #688.

I would be happy if I could improve the robustness of io.anserini.ltr.FeatureExtractorCli further by adding some integration-tests.
If there is some interest in that just let me know, I would be glad to help.

if ("clueweb".equals(collection)) {
return new WebxmlTopicReader(Paths.get(topicsFile));
}
else if ("gov2".equals(collection)){
Copy link
Member

Choose a reason for hiding this comment

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

we use two space indent, and else with preceding braces.

@lintool
Copy link
Member

lintool commented Jun 8, 2019

Thanks for your contribution!

Other than minor formatting changes, we can merge.

@lintool
Copy link
Member

lintool commented Jul 7, 2019

@mam10eks do we have an update on how this is going?

@mam10eks
Copy link
Contributor Author

mam10eks commented Jul 7, 2019

Hi @lintool,

I am still working on this.
I have found some related problems in the RankLibReranker and this motivated me to clean up both parts.

Since I am working on this in my spare time I think I need two more weeks until I can finish this and have everything properly tested.

My goal is to reproduce some results reported in the "Learning to Rank for Information Retrieval" book (Tie-Yan Liu) with the Anserini-LTR-Pipeline to be sure that everything is fine.

@MathBunny
Copy link
Contributor

@mam10eks How is the progress going? Are you still working on it?

@mam10eks
Copy link
Contributor Author

Hi @MathBunny,
sorry for the long delay.

I had planned too much and there have been some changes in the Master Branch since then.
I will fix those stuff tomorrow and then provide a small merge request instead of a large one.

@mam10eks
Copy link
Contributor Author

mam10eks commented Sep 17, 2019

@MathBunny,

From my side this can be merged now.

There are still other issues in the LTR-Pipeline.
Is anyone else working on that?
I still work on that in my spare time, but it needs some weeks (I will split this into "smaller merge requests").

@lintool
Copy link
Member

lintool commented Sep 17, 2019

hi @MathBunny can you take a look? you're more familiar with the code than I am at this point... do a CR and if it's okay we'll merge.

Copy link
Contributor

@MathBunny MathBunny left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@lintool lintool left a comment

Choose a reason for hiding this comment

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

will merge once Jenkins turns green

@lintool lintool merged commit 33d242f into castorini:master Sep 18, 2019
crystina-z pushed a commit to crystina-z/anserini that referenced this pull request Oct 28, 2022
Following castorini#692 - which fixes castorini#689 - changes need to be propagated to the documentation also.
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.

FeatureExtractorCli fails on collection clueweb
3 participants