style(model-based): fix mypy and polish api docstring #240

Gaiejj · 2023-05-21T14:59:29Z

Description

Clean useless argument.
Specify typing. For example: list -> list[float].
Fix api docstring. For example, some argument docs is np.ndarray but actually it is torch.Tensor.
Clear redundant code. Some methods simply unused, so I delete it.

Motivation and Context

Some codes and docs exist problems and need to be polished.
This pull request solves issue #230

I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds core functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (update in the documentation)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

I have read the CONTRIBUTION guide. (required)
My change requires a change to the documentation.
I have updated the tests accordingly. (required for a bug fix or a new feature)
I have updated the documentation accordingly.
I have reformatted the code using make format. (required)
I have checked the code using make lint. (required)
I have ensured make test pass. (required)

Gaiejj · 2023-05-21T15:03:22Z

omnisafe/adapter/modelbased_adapter.py

+
+

2 lines blank.

Gaiejj · 2023-05-21T15:03:57Z

omnisafe/adapter/modelbased_adapter.py

-        _env_id (str): The environment id.
-        _device (torch.device): The device.
-        _env (CMDP): The environment.
-        _cfgs (Config): The configuration.
-        _ep_ret (torch.Tensor): The episode return.
-        _ep_cost (torch.Tensor): The episode cost.
-        _ep_len (torch.Tensor): The episode length.
-        _last_dynamics_update (float): The last time of dynamics update.
-        _last_policy_update (float): The last time of policy update.
-        _last_eval (float): The last time of evaluation.


Private attributes is not need to be shown.

codecov-commenter · 2023-05-21T15:07:19Z

Codecov Report

Merging #240 (cf64aaf) into dev (d853ad8) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head cf64aaf differs from pull request most recent head f916927. Consider uploading reports for the commit f916927 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##              dev     #240      +/-   ##
==========================================
- Coverage   96.99%   96.98%   -0.01%     
==========================================
  Files         134      134              
  Lines        6867     6888      +21     
==========================================
+ Hits         6660     6680      +20     
- Misses        207      208       +1

Impacted Files	Coverage Δ
omnisafe/adapter/online_adapter.py	`100.00% <ø> (ø)`
omnisafe/algorithms/model_based/planner/cap.py	`100.00% <ø> (ø)`
omnisafe/algorithms/model_based/planner/cce.py	`100.00% <ø> (ø)`
omnisafe/algorithms/model_based/planner/rce.py	`93.33% <ø> (ø)`
...isafe/algorithms/on_policy/base/policy_gradient.py	`100.00% <ø> (ø)`
omnisafe/common/logger.py	`97.50% <ø> (ø)`
omnisafe/envs/core.py	`90.62% <ø> (ø)`
omnisafe/envs/mujoco_env.py	`93.33% <ø> (ø)`
omnisafe/envs/safety_gymnasium_env.py	`97.96% <ø> (ø)`
omnisafe/envs/wrapper.py	`98.63% <ø> (ø)`
... and 14 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Gaiejj · 2023-05-21T15:07:48Z

omnisafe/adapter/modelbased_adapter.py

+    def get_cost_from_obs_tensor(self, obs: torch.Tensor) -> torch.Tensor:
        """Get cost from tensor observation.

        Args:
-            obs (torch.Tensor): The observation.
+            obs (torch.Tensor): The tensor version of observation.
        """
        return (
            self._env.get_cost_from_obs_tensor(obs)
            if hasattr(self._env, 'get_cost_from_obs_tensor')
-            else None
+            else torch.zeros(1)
        )

-    def get_lidar_from_coordinate(self, obs: torch.Tensor) -> torch.Tensor | None:
+    def get_lidar_from_coordinate(self, obs: np.ndarray) -> torch.Tensor | None:


Original version None is pretty bad because there is no if else process to deal with None, here use zero tensor as default.

Gaiejj · 2023-05-21T15:08:16Z

omnisafe/adapter/modelbased_adapter.py

+
+        Keyword Args:
+            render_mode (str, optional): The render mode, ranging from 'human', 'rgb_array', 'rgb_array_list'.
+                Defaults to 'rgb_array'.
+            camera_name (str, optional): The camera name.
+            camera_id (int, optional): The camera id.
+            width (int, optional): The width of the rendered image. Defaults to 256.
+            height (int, optional): The height of the rendered image. Defaults to 256.
        """


A new style of **kwargs docstring.

Gaiejj · 2023-05-21T15:08:36Z

omnisafe/adapter/modelbased_adapter.py

@@ -262,23 +282,22 @@ def roll_out(  # pylint: disable=too-many-arguments,too-many-locals
                truncated,
                next_state,
                info,
-                action_info,


Unused arguments.

Gaiejj · 2023-05-21T15:14:59Z

omnisafe/adapter/modelbased_adapter.py

            )
            self._current_obs = next_state
            if terminated or truncated:
                self._log_metrics(logger)
                self._reset_log()
                self._current_obs, _ = self.reset()
                if algo_reset_func is not None:
-                    algo_reset_func(current_step)
+                    algo_reset_func()


Unused arguments

Gaiejj · 2023-05-21T15:16:05Z

omnisafe/algorithms/model_based/base/ensemble.py

-        if torch.is_tensor(data):
-            return (data - self._mean_t) / self._std_t
-        return (data - self._mean) / self._std
+        return (data - self._mean_t) / self._std_t


Unused branch.

Gaiejj · 2023-05-21T15:17:44Z

omnisafe/algorithms/model_based/base/loop.py

-        return action, info
+        return action


The info={} is useless.

Gaiejj · 2023-05-21T15:18:50Z

omnisafe/algorithms/model_based/base/pets.py

+        assert isinstance(delta_state, torch.Tensor), 'delta_state should be torch.Tensor'
+        inputs = torch.cat((state, action), -1)
+        inputs = torch.reshape(inputs, (inputs.shape[0], -1))
+
+        labels = torch.reshape(delta_state, (delta_state.shape[0], -1))
+        if self._cfgs.dynamics_cfgs.predict_reward:
+            labels = torch.cat(((torch.reshape(reward, (reward.shape[0], -1))), labels), -1)
+        if self._cfgs.dynamics_cfgs.predict_cost:
+            labels = torch.cat(((torch.reshape(cost, (cost.shape[0], -1))), labels), -1)
+        inputs = inputs.cpu().detach().numpy()
+        labels = labels.cpu().detach().numpy()


If the delta_state is not a tensor, something went wrong. So I think assert is better than if here.

…nto dev-typing

Gaiejj · 2023-05-22T16:51:28Z

omnisafe/adapter/modelbased_adapter.py

@@ -206,34 +224,46 @@ def _wrapper(
        if self._env.num_envs == 1:
            self._env = Unsqueeze(self._env, device=self._device)

-    def roll_out(  # pylint: disable=too-many-arguments,too-many-locals


rollout, instead of roll_out.

Gaiejj · 2023-05-24T11:51:02Z

omnisafe/adapter/modelbased_adapter.py

@@ -244,16 +274,14 @@ def roll_out(  # pylint: disable=too-many-arguments,too-many-locals

        epoch_steps = 0

-        while epoch_steps < roll_out_step and current_step < self._cfgs.train_cfgs.total_steps:
-            action, action_info = act_func(current_step, self._current_obs, self._env)


The return info is {}, which is useless.

omnisafe/adapter/modelbased_adapter.py

omnisafe/algorithms/model_based/base/ensemble.py

muchvo · 2023-05-24T13:37:07Z

omnisafe/algorithms/model_based/base/ensemble.py

-        self._predict_cost = predict_cost
+        self._state_size: int = state_size
+        self._reward_size: int = reward_size
+        self._cost_siz: int = cost_size


Suggested change

self._cost_siz: int = cost_size

self._cost_size: int = cost_size

Won't this cause exceptions in CI?

Well, I don't know neither but I fixed it.

muchvo · 2023-05-24T13:48:06Z

omnisafe/algorithms/model_based/base/ensemble.py

@@ -466,8 +581,9 @@ def train(
        holdout_inputs, holdout_labels = inputs[:num_holdout], labels[:num_holdout]
        self._ensemble_model.scaler.fit(train_inputs)

+        train_mse_losses = []


will this change the original code behavior?

I have considered it. Because original function use the train_mse_loss in the for loop, that will raise unbound error. I just log the total training loss to log, but still use loss in loop to determine whether to break. The performance curve seems it does not change original code behavior.

muchvo · 2023-05-24T13:55:50Z

omnisafe/algorithms/model_based/base/pets.py

-                labels = torch.cat(((torch.reshape(cost, (cost.shape[0], -1))), labels), -1)
-            inputs = inputs.cpu().detach().numpy()
-            labels = labels.cpu().detach().numpy()
-        assert not torch.is_tensor(inputs) and not torch.is_tensor(


is this assertion useless?

You can see in line 263 and 264 that the inputs and labels have been transferred to numpy.ndarray(), so i don't think this assertion makes any sense.

omnisafe/algorithms/model_based/base/pets.py

omnisafe/algorithms/model_based/planner/arc.py

omnisafe/envs/core.py

omnisafe/envs/safety_gymnasium_modelbased.py

…nto dev-typing

muchvo

LGTM.

This reverts commit 50ac0dd.

Gaiejj added 12 commits May 20, 2023 21:51

style(model-based): fix mypy

a528ac3

chore: clean the code

35d45a1

style(model-based): fix mypy

e7015dd

style(model-based): fix mypy

789eb89

style(model-based): fix mypy

eaeb955

style: fix mypy

bf518a7

style(modelbased_adapter): fix mypy

045ff05

style(modelbased_adapter, algorithms): fix mypy

8e6a679

docs(model-based): fix api docstring

dcaa4e2

docs(model-based): fix api docstring

9a4a4a4

chore: update test

81f49d8

chore: clean the code

839c117

Gaiejj added documentation Improvements or additions to documentation enhancement New feature or request algorithm Some issues about algorithm codestyle update codestyple labels May 21, 2023

Gaiejj commented May 21, 2023

View reviewed changes

omnisafe/adapter/modelbased_adapter.py

Comment on lines +16 to +17

Copy link

Member Author

Gaiejj May 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 lines blank.

Gaiejj commented May 21, 2023

View reviewed changes

Gaiejj added 3 commits May 22, 2023 07:16

docs(model-based): fix api docstring

e09b8cb

Merge branch 'dev-typing' of https://github.com/Gaiejj/omnisafe_zjy i…

40235f0

…nto dev-typing

chore: clean the code

f8124b5

Gaiejj commented May 22, 2023

View reviewed changes

Gaiejj marked this pull request as ready for review May 23, 2023 07:44

Gaiejj requested review from XuehaiPan and muchvo as code owners May 23, 2023 07:44

Gaiejj requested review from hdadong and zmsn-2077 as code owners May 23, 2023 07:44

Gaiejj added 2 commits May 24, 2023 17:19

docs: update model-based api

6292581

docs: fix spelling

9c4e57b

Gaiejj commented May 24, 2023

View reviewed changes

muchvo reviewed May 24, 2023

View reviewed changes

omnisafe/adapter/modelbased_adapter.py Outdated Show resolved Hide resolved

muchvo reviewed May 24, 2023

View reviewed changes

omnisafe/algorithms/model_based/base/ensemble.py Outdated Show resolved Hide resolved

muchvo reviewed May 24, 2023

View reviewed changes

omnisafe/algorithms/model_based/base/ensemble.py Outdated Show resolved Hide resolved

muchvo reviewed May 24, 2023

View reviewed changes

omnisafe/algorithms/model_based/base/pets.py Outdated Show resolved Hide resolved

muchvo reviewed May 24, 2023

View reviewed changes

omnisafe/algorithms/model_based/planner/arc.py Outdated Show resolved Hide resolved

muchvo reviewed May 24, 2023

View reviewed changes

omnisafe/envs/core.py Outdated Show resolved Hide resolved

muchvo reviewed May 24, 2023

View reviewed changes

omnisafe/envs/safety_gymnasium_modelbased.py Outdated Show resolved Hide resolved

Gaiejj added 5 commits May 24, 2023 23:51

docs: update model-based api

b75707e

Merge branch 'dev-typing' of https://github.com/Gaiejj/omnisafe_zjy i…

4bb2cb4

…nto dev-typing

docs: fix spelling

f916927

docs: update model-based api

0acc68c

chore: clean the code

9f9558b

muchvo approved these changes May 25, 2023

View reviewed changes

zmsn-2077 approved these changes May 26, 2023

View reviewed changes

Gaiejj merged commit 50ac0dd into PKU-Alignment:dev May 26, 2023
4 checks passed

Gaiejj added a commit that referenced this pull request May 26, 2023

Revert "style(model-based): fix mypy and polish api docstring (#240)"

4c21d4f

This reverts commit 50ac0dd.

Gaiejj deleted the dev-typing branch June 23, 2023 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style(model-based): fix mypy and polish api docstring #240

style(model-based): fix mypy and polish api docstring #240

Gaiejj commented May 21, 2023

Gaiejj May 21, 2023

Gaiejj May 21, 2023 •

edited

codecov-commenter commented May 21, 2023 •

edited

Gaiejj May 21, 2023

Gaiejj May 21, 2023

Gaiejj May 21, 2023

Gaiejj May 21, 2023

Gaiejj May 21, 2023

Gaiejj May 21, 2023

Gaiejj May 21, 2023

Gaiejj May 22, 2023

Gaiejj May 24, 2023

muchvo May 24, 2023

muchvo May 24, 2023

Gaiejj May 24, 2023 •

edited

muchvo May 24, 2023

Gaiejj May 24, 2023

muchvo May 24, 2023

Gaiejj May 24, 2023

muchvo left a comment

	self._cost_siz: int = cost_size
	self._cost_size: int = cost_size

style(model-based): fix mypy and polish api docstring #240

style(model-based): fix mypy and polish api docstring #240

Conversation

Gaiejj commented May 21, 2023

Description

Motivation and Context

Types of changes

Checklist

Choose a reason for hiding this comment

Gaiejj May 21, 2023 • edited

Choose a reason for hiding this comment

codecov-commenter commented May 21, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gaiejj May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muchvo left a comment

Choose a reason for hiding this comment

Gaiejj May 21, 2023 •

edited

codecov-commenter commented May 21, 2023 •

edited

Gaiejj May 24, 2023 •

edited