changed _departed_ids, and _arrived_ids in the update function #926
Conversation
This file contains hidden or 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
Yasharzf
requested review from
AboudyKreidieh,
cathywu,
eugenevinitsky and
kanaadp
as code owners
flow/core/kernel/vehicle/traci.py
Outdated
| """See parent class.""" | ||
| if len(self._arrived_ids) > 0: | ||
| return self._arrived_ids[-1] | ||
| return self._arrived_ids |
There was a problem hiding this comment.
why this change? this affects a lot of the codebase. Otherwise LGTM
There was a problem hiding this comment.
self._arrived_ids was a list storing ids for each step. in this function we just want the ids in the last step ([-1]). Since I don't have a list anymore, I deleted [-1].
There was a problem hiding this comment.
So if it's not a list anymore why does it have a len method?
flow/core/kernel/vehicle/traci.py
Outdated
| """See parent class.""" | ||
| if len(self._departed_ids) > 0: | ||
| return self._departed_ids[-1] | ||
| return self._departed_ids |
Pull Request Test Coverage Report for Build 5866
💛 - Coveralls |
Comment on lines
+214
to
+215
| self._num_departed.append(sim_obs[tc.VAR_LOADED_VEHICLES_NUMBER]) | ||
| self._num_arrived.append(sim_obs[tc.VAR_ARRIVED_VEHICLES_NUMBER]) |
There was a problem hiding this comment.
as long as there is a getter for these, this LGTM
brentgryffindor
pushed a commit
that referenced
this pull request
May 18, 2020
* changed _departed_ids, and _arrived_ids in the update function * fixed bug in get_departed_ids and get_arrived_ids
liljonnystyle
pushed a commit
that referenced
this pull request
May 26, 2020
* changed _departed_ids, and _arrived_ids in the update function * fixed bug in get_departed_ids and get_arrived_ids
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in 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.
Pull request information
Description
In vehicle/traci, we are storing all departed and arrived vehicle IDs for each simulation step. Every simulation step we are appending new vehicle list to _departed_ids and _arrived_ids. In large networks or high horizon times, this can be expensive and results in significant slow down. I changed _departed_ids and _arrived_ids to just store the current ids. I didn't totally delete them since we may need them (e.g., as in MultiEnv)