Conversation
AboudyKreidieh
requested review from
cathywu and
eugenevinitsky
as code owners
|
|
||
| self.visible.extend(lane_leaders) | ||
| self.visible.extend(lane_followers) | ||
| self.visible.extend([lane_follower]) |
There was a problem hiding this comment.
Are these changes just to more clearly show when the list of visible cars is being changed?
There was a problem hiding this comment.
this is to fix a bug where the list of visible vehicles sometimes includes '' strings
| eta = 8 # 0.25 | ||
| rl_actions = np.array([1]) | ||
| accel_threshold = 0 | ||
| np.tanh(np.mean(np.abs(rl_actions))) |
There was a problem hiding this comment.
is this line supposed to do anything?
There was a problem hiding this comment.
nice catch, will fix
flow/envs/base_env.py
Outdated
|
|
||
| # list of sorted ids (defaults to regular list of vehicle ids if the | ||
| # "sort_vehicles" attribute in env_params is set to False) | ||
| self.sorted_ids = deepcopy(self.vehicles.get_ids()) |
There was a problem hiding this comment.
This doesn't seem like it's sorted?
There was a problem hiding this comment.
good point, I'll call the method here as well
|
|
||
| class TestTestEnv(unittest.TestCase): | ||
|
|
||
| """Tests the TestEnv environment in flow/envs/test.py""" |
There was a problem hiding this comment.
This isn't a comment, but the name of this class is surreal.
| / self.env.scenario.max_speed, | ||
| self.env.get_x_by_id(veh_id) / | ||
| self.env.scenario.length] | ||
| for veh_id in self.env.sorted_ids]) |
There was a problem hiding this comment.
I sort of agree with Nish, what is the point of tests like these? This is just copying code and checking that the code you've copied is the same. Note, this is meant constructively; I'm sure I'm missing something.
There was a problem hiding this comment.
they're for two reasons:
- solidifying certain aspects of the codebase that we are not interested in developing on anymore. Personally for me, the environments feel very fragile as is.
- maintaining some level of confidence that an environment will train. since we cannot easily run RL tests to completion, we can at least ensure that the environments are still the same (I have lost a good deal of time trying to figure out what's changed in an old environment just to find out that I or someone else had introduced new bugs, probably when they were trying something locally)
generally speaking, tests for environments should cover more intricate methods, but the ones I tested were pretty generic, minus I guess testing the reset method for WaveAttenuationEnv
There was a problem hiding this comment.
Okay, I'm convinced of the value of this for now. We should replace these later with some sort of regression testing (TBD).
There was a problem hiding this comment.
So in conclusion, it's a saved version of the envs.
There was a problem hiding this comment.
yup, I was thinking the same about regression testing. these could just be a sanity test for now
|
Broadly, what's the point of these tests of the envs? It seems like the code is copied from the original env and then we check that the code has been copied correctly. |
| @property | ||
| def action_space(self): | ||
| return Box(low=0, high=0, shape=0, dtype=np.float32) | ||
| return Box(low=0, high=0, shape=(0,), dtype=np.float32) |
There was a problem hiding this comment.
This file should not be named test.py as it's a little confusing
| self.assertAlmostEqual(self.env.vehicles.get_speed("rl_0"), 0.1, 2) | ||
|
|
||
|
|
||
| class TestLaneChangeAccelEnv(unittest.TestCase): |
There was a problem hiding this comment.
If this test is specific to the loop, it's a little misnamed
eugenevinitsky
left a comment
eugenevinitsky
left a comment
There was a problem hiding this comment.
LGTM, see +1 comment as it is unaddressed I think.
3 participants
WaveAttenuationEnv)base_envand theVehiclesclass while writing the environment tests