-
Notifications
You must be signed in to change notification settings - Fork 51
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
PR curl encoder #1053
base: pytorch
Are you sure you want to change the base?
PR curl encoder #1053
Conversation
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.
Just some general comments. Not yet going to implementation detail yet.
max_episode_steps=1000, | ||
gym_env_wrappers=(), | ||
alf_env_wrappers=[AlfEnvironmentDMC2GYMWrapper], | ||
image_channel_first=False): |
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.
since the observation of dmc is known to be channel first. We don't need this option.
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import alf |
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.
Please add unittest suite_dmc_test to test its functionality and add it to .ci-cd/build.sh
@@ -0,0 +1,245 @@ | |||
# Copyright (c) 2021 Horizon Robotics and ALF Contributors. All Rights Reserved. |
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 better to rename this file as suite_dmc.py
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.
This is independent of CURL. So it should be submitted in a separate PR.
|
||
|
||
|
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.
Too many blank spaces
For installation of MuJoCo200, see https://roboti.us. | ||
Args: | ||
environment_name (str): Do not use this arg, this arg is here to | ||
metch up with create_environment. |
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.
metch => match
max_episode_steps (int): If None the max_episode_steps will be set to the | ||
default step limit defined in the environment's spec. No limit is applied | ||
if set to 0 or if there is no max_episode_steps set in the environment's | ||
spec. |
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.
This comment needs to be updated.
|
||
|
||
@alf.configurable | ||
def wrap_env(gym_env, |
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 looks exact same as the suite_gym.wrap_env. Should use that instead.
the end of encoder. | ||
|
||
Retrun: | ||
A CURL model. |
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.
no need to comment return for __init__
. It always return an instance of the class.
self._encoding_net = creat_encoder(self.after_crop_spec, feature_dim) | ||
self._target_encoding_net = self._encoding_net.copy( | ||
name='target_encoding_net_ctor') | ||
self.CrossEntropyLoss = nn.CrossEntropyLoss(reduction='none') |
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.
class member name is lower case words prefixed by _
No description provided.