This repository was archived by the owner on Dec 16, 2022. It is now read-only.
Fix quarel explanations #2648
Merged
Fix quarel explanations #2648
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brendan-ai2
approved these changes
Mar 26, 2019
| world_extractions, | ||
| answer_index, | ||
| world) | ||
| assert len(explanation) == 4 |
There was a problem hiding this comment.
Worth testing the structure of the explanations too?
There was a problem hiding this comment.
Perhaps, but not in this PR. There should have been more tests of the quarel code originally - the lack of tests was what made it so this wasn't caught earlier. But because things aren't changing much in this code, I'd prefer an add-as-needed approach to testing - I don't want to take the time to add exhaustive tests right now.
reiyw
pushed a commit
to reiyw/allennlp
that referenced
this pull request
Nov 12, 2019
* Fix a bug due to a change in the behavior of lisp_to_nested_expression * Add regression test that fails on master
TalSchuster
pushed a commit
to TalSchuster/allennlp-MultiLang
that referenced
this pull request
Feb 20, 2020
* Fix a bug due to a change in the behavior of lisp_to_nested_expression * Add regression test that fails on master
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was due to some refactoring that I did a while ago, and it is the cause of the broken QuaRel demo. I didn't see an easy way to test for this kind of breakage in the demo repository, unfortunately, but I added a test for the breakage here.
I'm not going to say that this fixes the issue in the demo repo (allenai/allennlp-demo#182), because the demo repo needs to be updated to use this commit after this gets merged, and then it will be fixed.