[LIGHT] Jericho World dataset #3957
Conversation
mojtaba-komeili
force-pushed
the
jericho-world
branch
2 times, most recently
from
6324b46 to
9d40e90
Compare
mojtaba-komeili
force-pushed
the
jericho-world
branch
from
77e2c07 to
155c7b9
Compare
There was a problem hiding this comment.
Overall this implementation is looking good - thanks for taking the time to break down the Jericho dataset and figure out what was not quite aligning. Did you come up with an answer to why some of the target actions weren't aligning with the given content?
I've left some nits, mostly about documentation for things that aren't super clear. You don't need to necessarily always be documenting at this level, but it certainly helps for getting code OSS ready early on, and makes it easier to return to this later when we need it again.
|
|
||
| # The set of words in the description of graph that we skip while checking | ||
| # for overlap between knowledge grapj vertices and the description. | ||
| GRAPH_VERT_SKIP_TOKEN = set(stopwords.words('english')) - {'you'} |
There was a problem hiding this comment.
Nit - it's useful to include reasonings for this kind of decision when external context drove the decision. Here the choice to leave out "you" is deliberate, but the note just describes what these tokens are used for.
There was a problem hiding this comment.
Added extra explanation.
| return True | ||
| return False | ||
|
|
||
| def keep_edge(edge, text_tokens): |
There was a problem hiding this comment.
I'm not sure I follow what your keep_edge heuristic is here - so we keep all 'you in X' and 'you have X', but otherwise we keep edges where the subject and object are in the text? Probably could use a quick one line desc to this effect.
'''
Returns true if all the components of the edge are in the given text, or if it is the player location or unpruned inventory
'''
Sure this documentation isn't required, but it makes it easier to grok what's happening.
There was a problem hiding this comment.
Explained it in details in the method docstring.
I couldn't figure out many of them, so ended up dropping them if them if they were detected as incomplete. But was able to "patch" some of them to some extent by checking the step before/after. I reached out to them about clarifying, but haven't received any response yet. |
Patch description
Added ParlAI teachers for the JerichoWorld Dataset dataset. There are two sets of teachers for predicting (1) knowledge graph (2) actions. The focus of this patch is on knowledge graph teachers; although a simple action teacher is added as well. Here is a list of main teachers that are expected to use (not abstract etc.):
StaticKGTeacher: predict knowledge graph for a single state
ActionKGTeacher predicts mutations to the knowledge graph after an action
StateToActionTeacher: predicts player actions given the observation
