Hotfix winning conditions #274
Conversation
- mirrored implementation from win condition (`generator/game.py:563`)
- Fix description to fail_conditions functions and rename win_condition(...) -> win_conditions(...), fail_condition(...) -> fail_conditions(...) for more accurate sense of return values
|
Thanks for the PR. Can I ask what is your use case for those functions? Since there are not used anywhere else in the code, I'm very tempted to simply delete them, actually. If you do have a use case, proper tests should be added. |
textworld/generator/game.py
Outdated
| @@ -560,9 +560,14 @@ def verbs(self) -> List[str]: | |||
| return sorted(set(cmd.split()[0] for cmd in self.command_templates)) | |||
|
|
|||
| @property | |||
| def win_condition(self) -> List[Collection[Proposition]]: | |||
| def win_conditions(self) -> List[Collection[Proposition]]: | |||
There was a problem hiding this comment.
The typing needs to be changed too. I.e., List[Tuple[Event]].
textworld/generator/game.py
Outdated
| return [q.win_events for q in self.quests] | ||
|
|
||
| @property | ||
| def fail_conditions(self) -> List[Collection[Proposition]]: |
There was a problem hiding this comment.
The typing needs to be changed too. I.e., List[Tuple[Event]].
My use case lies in some work I'm doing relating to goal-conditioned RL. Having ground truth propositional logic of goal states is a required feature. As far as I could investigate, this was is the only way to retrieve that information (please correct me if I'm wrong). Although the propositional logic return by these functions uses the game's object IDs to instead of their names, so a second ID-> name lookup is actually needed, but that's just an aside. With this second lookup, I can compare against the game's Regarding tests, were there any specific guidelines that needed to be followed? My use case is just as information retrieval. |
|
@MathieuTuli here's what I suggest: MathieuTuli#1 |
Add win_facts and fail_facts to EnvInfos.
Agreed, I had this in mind but opted for the quick workaround, which in hindsight was the wrong decision. Merged your PR, changes should be reflected. |
|
Alright. Waiting for all the tests to pass, then I will make a minor release. Thank you for suggesting this new feature. |
|
I just noticed the PEP8 check has failed. Here's the fix: MathieuTuli#2 |
…utuli/TextWorld into hotfix-winning-conditions
I had noticed it too, made the change directly just before you mentioned this. PEP8 should be happy now |
|
Just a quick update. It seems there's an issue fetching packages from inform7.com website for running the tests on Azure pipelines. I'll check tomorrow to see if that's still an issue. |
|
@MathieuTuli FYI TextWorld 1.4.4 is now released on PyPi. |
The
win_conditionproperty as part of thegenerator.game.Gameclass (found intextworld/generator/game.py) was calling the wrong member variable for the quests. This PR fixes this by calling[Quest].win_events, not[Quest].winning_conditionsas it previously was.Further, mirrored the
win_conditionproperty to create the analogousfail_conditionproperty, if that info is ever needed by the user.Lastly, renamed
win_condition->win_conditionsandfail_condition->fail_conditionsto more accurately represent their return values, which are a list of conditions (which are in fact,generator.game.Events)Note (Sanity Check):
winning_conditionsisn't a member variable that exists, and only appears in this one file in the whole code base.