-
Notifications
You must be signed in to change notification settings - Fork 14
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
Rjf/determinism #203
Rjf/determinism #203
Conversation
@nreinicke i think i've got it! can you run |
Just pulled and ran and it looks like it's checking out! finished iteration 4
mean_final_soc is good, all values match
requests_served_percent is good, all values match
total_vkt is good, all values match
total_kwh_expended is good, all values match
total_gge_expended is good, all values match
total_kwh_dispensed is good, all values match
total_gge_dispensed is good, all values match What was the main cause? |
maybe a terrible idea, but, i set up the github actions to call the determinism test at PR. it will run denver demo twice and confirm high-level metrics match. that'll slow us down a little but maybe worth it to make sure we don't backslide on determinism. |
i went beyond SimulationState and did a search for vs: List[Vehicle] = ... #
sorted(vs, key=lambda v: v.distance_traveled_km) # bad
sorted(vs, key=lambda v: (v.distance_traveled_km, v.id)) # good that's a problem if two vehicles had traveled the same distance, there's nothing to determine what order those two will fall into. gets worse when the sort is by "queue time", those are going to be integers, and so anytime two agents have the same queue time, there's no tiebreaker to use as a second-tier sort. i went through and added a similar problem would happen with calls to get_{vehicles|stations|bases|requests} methods that supplied sort_keys. and then there were just a few other iterations that didn't sort by id in absence of any requested sort values. tl;dr: sortsortsortsortsortsortsortsortsortsortsortsortsortsortsort |
Aha that makes a lot of sense, nice work getting deep into this one to figure it out!! With respect to the determinism test in the action I think that's a good idea. |
@nreinicke the determinism check passed, yay! it replaces the 'hive denver_demo.yaml' action. ready for your review. |
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.
Looks great!
some efforts here to address non-determinism in hive. still seeing it after these changes. for now,
any thoughts on what else i could do here?
edit: i just went through and added a ton more to this. details:
i was able to observe 50 denver_demo runs in a row that had the same result. i'm also seeing those runs take 15 seconds which i believe is a bit longer than before. we may see a bigger performance hit with larger runs.
note: when i run
hive
at the command line twice in a row, i'm still seeing different results still. maybe we're not controlling for randomness as well when calling it there for some reason. to provide an example, i've addedexamples/test_for_determinism.py
which runs a quick set of 5 iterations of denver and confirms whether all results match or not.