Skip to content
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

Open
wants to merge 3 commits into
base: pytorch
Choose a base branch
from
Open

PR curl encoder #1053

wants to merge 3 commits into from

Conversation

7Gao
Copy link
Contributor

@7Gao 7Gao commented Nov 3, 2021

No description provided.

Copy link
Contributor

@emailweixu emailweixu left a 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):
Copy link
Contributor

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
Copy link
Contributor

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.
Copy link
Contributor

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

Copy link
Contributor

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.

Comment on lines +141 to +143



Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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,
Copy link
Contributor

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.
Copy link
Contributor

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')
Copy link
Contributor

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 _

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants