{NREL#146}_fix_stack_overflow_error_time_to_fill#173
Conversation
robfitzgerald
left a comment
robfitzgerald
left a comment
There was a problem hiding this comment.
thanks for taking the initiative to fix the actual problem here. a few requests, particularly to expose the default via HiveConfig. thanks! 🐝
|
|
||
|
|
||
| class TestPowercurveOps(TestCase): | ||
| def test_time_to_fill(self): |
There was a problem hiding this comment.
give it a name that implies what is being tested, something like test_time_to_fill_when_near_full
There was a problem hiding this comment.
Agreed, fixed
| charger: Charger, | ||
| target_soc: Ratio, | ||
| sim_timestep_duration_seconds: Seconds, | ||
| min_delta_energy_change: Ratio = 0.0001 |
There was a problem hiding this comment.
looks great. to set this default value, could you:
- pass this default in from here
- everywhere it's called, for example here, you could grab it from the environment (see below for example)
- default configuration is set in the default hive.yaml file, you can add the row
min_delta_energy_change: 0.0001after schedule_type.
min_delta_energy_change = env.config.sim.min_delta_energy_change| time_charged = _fill(vehicle) | ||
| vehicle, time_delta = mechatronics.add_energy(vehicle, charger, sim_timestep_duration_seconds) | ||
|
|
||
| delta = abs(prev_energy - vehicle.energy.get(charger.energy_type)) / prev_energy |
There was a problem hiding this comment.
ooh, one more thing i just noticed. we need to catch the case that prev_energy is not zero, which is possible if the user sets a vehicles.csv entry at 0% soc with at-home charging. please include that along with a unit test. and that covers at least those extrema edge cases of [0,1] values i think.
to deal with the grid throttle scenario, what might be a good thing in the future would be to pass some max charge time argument. that at least can be set to int((sim.end_time - sim.sim_time) / sim.timestep_duration_seconds) .
There was a problem hiding this comment.
Agreed, I'll make a comment in the doc string and we can make a new issue
clintonsteiner
force-pushed
the
NREL#146_Stack_overflow_error_time_to_fill
branch
from
2016911 to
ca5b569
Compare
|
Updated commit, I think I addressed all changes? |
clintonsteiner
force-pushed
the
NREL#146_Stack_overflow_error_time_to_fill
branch
3 times, most recently
from
e212a45 to
c9f833a
Compare
clintonsteiner
force-pushed
the
NREL#146_Stack_overflow_error_time_to_fill
branch
from
c9f833a to
169ef6a
Compare
nreinicke
left a comment
nreinicke
left a comment
There was a problem hiding this comment.
This looks good, thanks for addressing those changes.
3 participants
No description provided.