Conversation
AboudyKreidieh
requested review from
cathywu,
eugenevinitsky and
kanaadp
as code owners
Pull Request Test Coverage Report for Build 2061
💛 - Coveralls |
flow/core/kernel/kernel.py
Outdated
| self.traffic_light.pass_api(kernel_api) | ||
|
|
||
| def update(self, reset): | ||
| """Update the kernel subclasses with after a simulation step. |
There was a problem hiding this comment.
Should this be: "Update the kernel subclasses after a simulation step"
|
|
||
| >>> k = Kernel(simulator="...") # a kernel for some simulator type | ||
| >>> veh_id = "..." # some vehicle ID | ||
| >>> k.vehicle.apply_acceleration(veh_id) |
There was a problem hiding this comment.
apply_acceleration is moving to the vehicle class? Which class subclasses GymEnv?
There was a problem hiding this comment.
Also, not sure if I agree with the variable name k for the kernel. While it's abundantly clear to us what it means, it may add additional confusion for new developers.
There was a problem hiding this comment.
So the gym env will still be in base env, with all the relevant calls available from there. What this is meant to do is when you pass a command to the simulator, the command isn't sent to a method within the gym class, but rather to a method within the correct simulation kernel. That way apply_acceleration can work for any simulator without us having to modify the envs.
There was a problem hiding this comment.
In terms of k, we'll have documentation on this, but if you'd rather we call it something else, e.g. self.kernel, I'd be down too. It just makes calling commands longer
| format(simulator)) | ||
|
|
||
| def pass_api(self, kernel_api): | ||
| """Pass the kernel API to all kernel subclasses.""" |
There was a problem hiding this comment.
Shouldn't this be called in the init? Otherwise where do they get access to this?
There was a problem hiding this comment.
Init actually is going to consist of three stages:
- create the scenario files
- start the simulation and generate the kernel api
- pass the kernel api to the kernel and all kernel subclasses
| """ | ||
| raise NotImplementedError | ||
|
|
||
| def prev_edge(self, edge, lane): |
There was a problem hiding this comment.
Since we're redoing this right now, we may want to discuss what the appropriate thing to do is when two edges converge at a single edge
There was a problem hiding this comment.
sure! this is just architecturally what things will look like, but we can have this discussion when we start adding actual code
| @@ -0,0 +1,85 @@ | |||
| """Script containing the base simulation kernel class.""" | |||
|
|
|||
There was a problem hiding this comment.
Repeat; does this become the gym Env?
|
Looks good but we should discuss the raised issues. |
|
Okay, this looks fine to me. I'm approving it and you can merge it when you're ready. |
|
Sounds good! The issues will be thoroughly addressed when we get to the actual code. But the concerns are noted and I'll keep them in mind as we move forward. |
4 participants
This PR contains all abstractions to the kernel class, and is meant to give a sense of the overall architecture of what is to come. All methods current raise
NotImplmenetedError, and the bulk of the changes are actually documentation. The person reading this is simply meant to read the docstrings and comment on the chose of design. In its current form, the kernel consists of four "sub-kernels" covering simulations, scenarios, traffic lights, and kernels. More details can be found within the PR itself.