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 trec_eval Fix #475 #477

Merged
merged 4 commits into from Nov 16, 2018

Conversation

Projects
None yet
3 participants
@Chriskamphuis
Copy link
Contributor

Chriskamphuis commented Nov 16, 2018

Updated trec_eval to latest version. Running old version can result in a
segmentation fault.

Update trec_eval Fix #475
Updated trec_eval to latest version. Running old version can result in a
segmentation fault.
@lintool

This comment has been minimized.

Copy link
Member

lintool commented Nov 16, 2018

Hi @Chriskamphuis I only see removal of the existing tarball? There isn't a new one?

@Chriskamphuis

This comment has been minimized.

Copy link
Contributor Author

Chriskamphuis commented Nov 16, 2018

Hi @lintool, I replaced it with a tarball with the same name. The commit shows that the file is modified (not deleted).

@lintool

This comment has been minimized.

Copy link
Member

lintool commented Nov 16, 2018

Ah, I see - misread.

The new tarball is created from https://github.com/usnistgov/trec_eval/ ?
I think it would reduce confusion to change the name? I think the latest version is 9.0.4? Might want to include the GitHub commit id in this PR to be absolutely clear.

Renamed filename of tarball
Tarball is created from usnistgov/trec_eval with
a5211566d0c9e2ec337bacf327b9350ab5b3edde as latest commit.
@Chriskamphuis

This comment has been minimized.

Copy link
Contributor Author

Chriskamphuis commented Nov 16, 2018

@lintool

I included the commit id in the commit that changed the name, the commit id of usnistgov/trec_eval is a5211566d0c9e2ec337bacf327b9350ab5b3edde.

@lintool

This comment has been minimized.

Copy link
Member

lintool commented Nov 16, 2018

@Chriskamphuis sorry to be kinda a pain, but we're try to implement rigorous software engineering practices in this project - CI is failing because of this -

https://github.com/castorini/Anserini/blob/master/.travis.yml#L16

File name needs to be updated. Can you please do so?

@Chriskamphuis

This comment has been minimized.

Copy link
Contributor Author

Chriskamphuis commented Nov 16, 2018

@lintool Yes, no problem, I am working on it. I will also update the markdown files so it will be consistent.

Rename trec_eval.9.0 to trec_eval.9.0.4
Renamed in all relevant files
@@ -60,47 +60,47 @@ nohup target/appassembler/bin/SearchCollection -topicreader Webxml -index lucene
Evaluation can be performed using `trec_eval` and `gdeval.pl`:

```
eval/trec_eval.9.0/trec_eval -m map -m P.30 src/main/resources/topics-and-qrels/qrels.web.51-100.txt run.cw09b.bm25.topics.web.51-100.txt

This comment has been minimized.

@Peilin-Yang

Peilin-Yang Nov 16, 2018

Collaborator

It is a bit odd why there is an extra empty line here.
Did you manually change this file?

This comment has been minimized.

@lintool

lintool Nov 16, 2018

Member

@Peilin-Yang are those parts auto-gen'ed? Perhaps @Chriskamphuis doesn't know about the details of the regression framework...

This comment has been minimized.

@Peilin-Yang

Peilin-Yang Nov 16, 2018

Collaborator

All experiments-{$collection}.md files are auto-generated.
@Chriskamphuis Could you please run mvn test locally and see if there is any diff?

This comment has been minimized.

@Chriskamphuis

Chriskamphuis Nov 16, 2018

Author Contributor

@Peilin-Yang No diff for me locally. I only changed the the index.md and yaml files. Then I ran test before pushing the changes. Maybe I accidentally added an extra line when checking if everything was correct, but then it should be removed when running mvn test right?

@lintool lintool merged commit b4064da into castorini:master Nov 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lintool

This comment has been minimized.

Copy link
Member

lintool commented Nov 16, 2018

@Chriskamphuis thanks for your first contribution. Looking forward to many more in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment