feat: improve test coverage and clear redundant code. #238
+305
−184
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gaiejj
added
enhancement
Gaiejj
changed the title
Gaiejj
commented
May 13, 2023
Comment on lines
+163
to
+165
| if self.bias is not None: | ||
| return torch.add(w_times_x, self.bias[:, None, :]) # w times x + b | ||
| return w_times_x # type: ignore |
There was a problem hiding this comment.
The original code exists some error.
If self.bias is None, then return torch.add(w_times_x, self.bias[:, None, :]) will take a slice of None. So I use an if else here.
Then, as mypy thinks that the code will not be reached, I wonder line 165 may be used in future model-based code, so I just disable mypy here.
Gaiejj
commented
May 13, 2023
Comment on lines
-415
to
-431
|
|
||
| def _log_when_not_update(self) -> None: | ||
| """Log when not update.""" | ||
| self._logger.store( | ||
| **{ | ||
| 'Loss/Loss_reward_critic': 0.0, | ||
| 'Loss/Loss_pi': 0.0, | ||
| 'Value/reward_critic': 0.0, | ||
| }, | ||
| ) | ||
| if self._cfgs.algo_cfgs.use_cost: | ||
| self._logger.store( | ||
| **{ | ||
| 'Loss/Loss_cost_critic': 0.0, | ||
| 'Value/cost_critic': 0.0, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Simply unused, so I delete, the same as others.
Gaiejj
commented
May 13, 2023
Comment on lines
+93
to
+102
| 'SafetyPointGoal1-v0_data_test': OfflineMeta( | ||
| url='https://drive.google.com/file/d/1JPJ127bWM_Tdej0AEGoFAqFFG9mWtzsN/view?usp=share_link', | ||
| sha256sum='417b580cd4ef8f05a66d54c5d996b35a23a0e6c8ff8bae06807313a638df2dc6', | ||
| episode_length=1, | ||
| ), | ||
| 'SafetyPointGoal1-v0_data_init_test': OfflineMeta( | ||
| url='https://drive.google.com/file/d/1WlfkoUvWuFUYVMlGwi_EdGO914oWndpV/view?usp=share_link', | ||
| sha256sum='fce6cc1fd0c294a8b66397f2f5276c9e7055821ded1f3a6e58e491eb342b1fbe', | ||
| episode_length=1, | ||
| ), |
There was a problem hiding this comment.
The test dataset to test offline dataset. Each dataset is only 0.1MB.
Gaiejj
commented
May 13, 2023
Comment on lines
+87
to
+106
| with open( | ||
| os.path.join( | ||
| f'{custom_cfgs["logger_cfgs"]["log_dir"]}', | ||
| terminal_log_name, | ||
| ), | ||
| 'w', | ||
| encoding='utf-8', | ||
| ) | ||
| # pylint: disable-next=consider-using-with | ||
| sys.stderr = open( # noqa: SIM115 | ||
| os.path.join(f'{custom_cfgs["logger_cfgs"]["log_dir"]}', error_log_name), | ||
| 'w', | ||
| encoding='utf-8', | ||
| ) | ||
| agent = omnisafe.Agent(algo, env_id, custom_cfgs=custom_cfgs) | ||
| reward, cost, ep_len = agent.learn() | ||
| ) as f_out: | ||
| sys.stdout = f_out | ||
| with open( | ||
| os.path.join( | ||
| f'{custom_cfgs["logger_cfgs"]["log_dir"]}', | ||
| error_log_name, | ||
| ), | ||
| 'w', | ||
| encoding='utf-8', | ||
| ) as f_error: | ||
| sys.stderr = f_error | ||
| agent = omnisafe.Agent(algo, env_id, custom_cfgs=custom_cfgs) | ||
| reward, cost, ep_len = agent.learn() |
There was a problem hiding this comment.
Use with open instead of previous version to meet pylint requirements.
Gaiejj
commented
May 13, 2023
| @@ -225,6 +224,7 @@ def test_cem_based(algo): | |||
| } | |||
| agent = omnisafe.Agent(algo, env_id, custom_cfgs=custom_cfgs) | |||
| agent.learn() | |||
| agent.render() | |||
There was a problem hiding this comment.
As model-based algorithms have many branches when evaluating, I render all of them to make a trust-worthy test.
Gaiejj
requested review from
XuehaiPan,
muchvo,
hdadong and
zmsn-2077
as code owners
Gaiejj
commented
May 14, 2023
Comment on lines
-250
to
-251
| except yaml.YAMLError as exc: | ||
| raise AssertionError(f'{path} error: {exc}') from exc |
There was a problem hiding this comment.
Actually, here will raise FileNotFoundError instead of yaml.YAMLError.
2 tasks
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
Close issue #237
Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:Checklist
Go over all the following points, and put an
xin all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!
make format. (required)make lint. (required)make testpass. (required)