Glulx game logger #19
Conversation
|
@taodav you are right the GameLogger class should implement a |
|
@MarcCote what would the |
| def reset(self) -> GameState: | ||
| """ | ||
| Reset the environment. | ||
| Also clears logs. |
There was a problem hiding this comment.
Didn't we say we are keeping multiple trajectories? Calling reset should append to a list of logs.
| Reset the environment. | ||
| Also clears logs. | ||
| """ | ||
| self._logs = [] |
There was a problem hiding this comment.
Member variables should be first declared in the __init__. That way, one can easily see what are the member variables at first glance. It also helps minimizing side-effects to some extent (e.g. logger.logs would fail before a call to env.reset() is made!).
| def add_commands(self, commands: List[str], scores: Optional[Union[Iterable[float], Sized]]=None) -> None: | ||
| """ | ||
| Add custom commands to the logger. Optionally add scores for each command. | ||
| :param commands: |
There was a problem hiding this comment.
We are using Google docstring standard now (https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html). See, core.py for examples on how to format them.
| assert len(scores) == len(commands) | ||
| command_mapping = {a: p for a, p in zip(commands, scores)} | ||
|
|
||
| self._current['command_distribution'] = command_mapping |
There was a problem hiding this comment.
We don't know if the scores are normalized, so I would avoid using the term distribution. Also, if scores is None, then it's not a mapping. We could simply store commands and scores separately and it would be up to the viewer/user to deal with it accordingly.
| Get all logs | ||
| :return: List of all logs | ||
| """ | ||
| logs = self._logs[:] |
There was a problem hiding this comment.
What's the purpose of this line? Make a shallow copy? If so, why do you append to it before returning it?
| return logs | ||
|
|
||
| @property | ||
| def gamefile(self): |
There was a problem hiding this comment.
I don't think you need this method. If I remember correctly, Wrappers instances automatically call method of the unwrapped environment if not defined/overwritten in the wrapper.
There was a problem hiding this comment.
Sorry what do you mean exactly? I don't see this in the Wrapper class.
There was a problem hiding this comment.
Nevermind, I was thinking of some unmerged code.
|
|
||
| if index < len(self._logs) - 1: | ||
| return self._logs[index] | ||
| return self._current |
There was a problem hiding this comment.
Why is _current not always the last element of _logs?
| Get serialized mappings of logs. | ||
| :return: List of serialized mappings. | ||
| """ | ||
| return self.logs |
There was a problem hiding this comment.
Missing newline at the end of the file (PEP8).
|
@taodav good point. I guess we should have a separate |
|
@MarcCote Perhaps |
|
@tavianator Haha, that was too obvious. Thanks 👍 |
|
@MarcCote refactored the game logs into a separate class! Also fixed the comments you mentioned above. |
textworld/envs/glulx/__init__.py
Outdated
| @@ -0,0 +1 @@ | |||
|
|
|||
| @@ -0,0 +1,268 @@ | |||
| # Copyright (c) Microsoft Corporation. All rights reserved. | |||
| # Licensed under the MIT license. | |||
| import json | |||
There was a problem hiding this comment.
I think we need a blank space after the license.
| class GameLog: | ||
| def __init__(self): | ||
| """ | ||
| GameLog object. Allows your to load and save previous game logs. |
There was a problem hiding this comment.
your -> you
Also, put this docstring at the class level instead of the init method.
| GameLog object. Allows your to load and save previous game logs. | ||
| """ | ||
| self._logs = [[]] | ||
| self._current_game = self._logs[-1] |
There was a problem hiding this comment.
No needed, see my comments on current_game property.
| Gets current game we're logging. | ||
| Returns: list of logs from current game. | ||
| """ | ||
| return self._current_game |
There was a problem hiding this comment.
Instead of having a private variable _current_game simple make this property return return self._logs[-1]
|
|
||
| return game_state | ||
|
|
||
| def add_commands(self, commands: List[str], scores: Optional[Union[Iterable[float], Sized]]=None) -> None: |
There was a problem hiding this comment.
Why the Union? Isn't Iterable[float] enough?
| self.serialized_game = env.game.serialize() | ||
| self._gamefile = env.gamefile | ||
|
|
||
| self._logs = GameLog() |
There was a problem hiding this comment.
Let's called this variable self._game_log
| """ | ||
| Appends optional information to current game | ||
| Args: | ||
| value: Value to append |
There was a problem hiding this comment.
Maybe mention value needs to be JSON serializable.
| """ | ||
| return self._gamefile | ||
|
|
||
| def __getitem__(self, index: int) -> Mapping: |
There was a problem hiding this comment.
Not sure we need this anymore.
| """ | ||
| return self._logs.logs | ||
|
|
||
| def save(self, filename) -> None: |
There was a problem hiding this comment.
I don't think we need save/load/serialize anymore. Users can call save directly on the gamelog object, i.e. env.game_log.save()
|
@MarcCote fixed all the above comments! |
There was a problem hiding this comment.
@tavianator do you think we could set up something like https://codecov.io/ to report code coverage and make sure it is high enough when accepting a PR?
| self._filename = '' | ||
|
|
||
| def __getitem__(self, idx: int) -> list: | ||
| """ | ||
| """A |
| Returns: log object for current game. | ||
| """ | ||
| if len(self._current_game) > 0: | ||
| if len(self.current_game) > 0: |
There was a problem hiding this comment.
What do you need this check?
There was a problem hiding this comment.
We need this check to see if the current game is already new. We initialize our logs with a game inside, so there would be a redundancy if we didn't do this check.
|
Also, @tavianator did you say you had an idea for solving the |
|
@MarcCote I'll look into codecov.io. In the meantime you can check the Travis logs, they still print out that coverage table at the end. I'm not sure about the connection reset error, but I'll take a look. |
Working game logger for the GitGlulxMLGameState.
A few things I've been thinking about for this feature:
.save()method that saves to a JSON file?