-
Notifications
You must be signed in to change notification settings - Fork 21
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
Fix Sdf Version Warning and LogRecord Er #110
Conversation
gazebo::util::LogRecord::Instance()->Start(params); | ||
gazebo::util::LogRecord::Instance()->Stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should stop it immediately, otherwise more warnings and errors will be generated by LogRecord.
This could also prevent memory growing. I did some test, it does not matter call Init() and Stop() or not, as long as Start() is called, the memory stop growing.
@@ -125,7 +125,7 @@ def __init__(self, | |||
self._action_cost = action_cost | |||
self._with_language = with_language | |||
self._seq_length = vocab_sequence_length | |||
self._with_agent_language = with_agent_language | |||
self._with_agent_language = with_language and with_agent_language |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems we should add this logic to prevent the case with_language is false and with_agent_language is true.
Perhaps we can change them to something like with_language_in_observation and with_language_in_action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix Jiangtao, LG. If we have more than 2 sources of language then at that point we can do _in_obs, _in_action etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG, thanks for the fixes!
@@ -260,11 +260,13 @@ def main(): | |||
import matplotlib.pyplot as plt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a separate play_ground_test.py, maybe we can remove this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still useful when we want render, test specific case, or run for a long time to test memory growth or something. Let's keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG, thanks for the fixes!
Fix some warning of #109
Changes:
Fix Sdf Version Warning
Fix LogRecord Er
Also fixed the test of play ground because of the new option with_agent_language.