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

Adding DatasetReader for the bAbI tasks and Qangaroo #2194

Merged
merged 18 commits into from Dec 18, 2018

Conversation

Projects
None yet
2 participants
@nicola-decao
Copy link
Contributor

commented Dec 17, 2018

Hi,

I would like to add a DatasetReader for the bAbI tasks and Qangaroo.
The bAbI reader considers the format used here https://github.com/facebook/bAbI-tasks, therefore, the official one. The Qangaroo reader takes JSON from here http://qangaroo.cs.ucl.ac.uk.

Nicola De Cao and others added some commits Dec 17, 2018

@nicola-decao nicola-decao changed the title Adding DatasetReader for the bAbI tasks Adding DatasetReader for the bAbI tasks and Qangaroo Dec 17, 2018

@matt-gardner

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

Thanks for the PR! In order for us to merge this, though, it needs to have tests. You can look at the other dataset readers to see what tests we have for them. There are also some style and type problems that TeamCity is bringing up, which you can see here.

@nicola-decao

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

Cool! Thanks for the advice. I am fixing the issues and writing the tests.

@nicola-decao

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

Hi, I added test and corrected all the type checking.

The only 2 warnings that I cannot fix are:

[18:36:53][Step 4/10] ************* Module allennlp.data.dataset_readers.babi
[18:36:53][Step 4/10] W: 72, 4: Parameters differ from overridden 'text_to_instance' method (arguments-differ)
[18:36:53][Step 4/10] ************* Module allennlp.data.dataset_readers.reading_comprehension.qangaroo
[18:36:53][Step 4/10] W: 65, 4: Parameters differ from overridden 'text_to_instance' method (arguments-differ)

I don't know why since it seems the same as other DataReader classes.

@matt-gardner Can you help?

@matt-gardner

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

We ignore those warnings:

# pylint: disable=arguments-differ

nicola-decao added some commits Dec 18, 2018

@matt-gardner
Copy link
Member

left a comment

Looks great! Just a few minor style / documentation fixes, and this is good to merge.

Show resolved Hide resolved allennlp/data/dataset_readers/babi.py Outdated
Show resolved Hide resolved allennlp/data/dataset_readers/babi.py
Show resolved Hide resolved allennlp/data/dataset_readers/babi.py Outdated
Show resolved Hide resolved allennlp/data/dataset_readers/reading_comprehension/qangaroo.py Outdated
Show resolved Hide resolved allennlp/data/dataset_readers/reading_comprehension/qangaroo.py Outdated
@matt-gardner

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

Thanks @nicola-decao!

@matt-gardner matt-gardner merged commit 6aaf76a into allenai:master Dec 18, 2018

3 checks passed

Pull Requests (AllenNLP Library) TeamCity build finished
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project 92% (+<1%) compared to a2084fd
Details
@nicola-decao

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

I'm glad to contribute 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.