-
Notifications
You must be signed in to change notification settings - Fork 378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add MPG reward #931
Add MPG reward #931
Conversation
72a93e3
to
429df10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in addition to the comments, docstring style should be fixed. pydocstyle test fails
"""Calculate power consumption of a vehicle. | ||
Assumes vehicle is an average sized vehicle. | ||
The power calculated here is the lower bound of the actual power consumed | ||
by a vehicle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add description of parameters and returns
|
||
accel = abs(speed - prev_speed) / env.sim_step | ||
|
||
power += M * speed * accel + M * g * Cr * speed + 0.5 * rho * A * Ca * speed ** 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have a reference especially for some of the coefficients (e.g., Cr)? if yes, you can add it to the docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they're from Joy and are custom values
"""Calculate miles per mega-joule of either a particular vehicle or the total average of all the vehilces. | ||
Assumes vehicle is an average sized vehicle. | ||
The power calculated here is the lower bound of the actual power consumed | ||
by a vehicle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params, returns?
flow/core/rewards.py
Outdated
# meters / joule is (v * \delta t) / (power * \delta t) | ||
mpj = speed / power | ||
else: | ||
for veh_id in env.k.vehicle.get_ids(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't you iterate over vehicle.get_ids() if veh_id is None? here veh_id can be a list, shouldn't you iterate over veh_id list?
"""Calculate mpg of either a particular vehicle or the total average of all the vehilces. | ||
Assumes vehicle is an average sized vehicle. | ||
The power calculated here is the lower bound of the actual power consumed | ||
by a vehicle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params and returns?
mpj = 0 | ||
counter = 0 | ||
if not isinstance(veh_id, list): | ||
speed = env.k.vehicle.get_speed(veh_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if veh_id is None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that'd be a higher up error. I can add an assert
mpg = 0 | ||
counter = 0 | ||
if not isinstance(veh_id, list): | ||
speed = env.k.vehicle.get_speed(veh_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, if veh_id is None?
flow/core/rewards.py
Outdated
# meters / gallon is (v * \delta t) / (gallons_s * \delta t) | ||
mpg = speed / gallons_per_s | ||
else: | ||
for veh_id in env.k.vehicle.get_ids(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
Pull Request Test Coverage Report for Build 5929
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Adds an MPG and MP-joule reward