integrate census scenario #426
Conversation
sim/src/make/scenario.rs
Outdated
| @@ -145,7 +145,8 @@ impl Scenario { | |||
| timer.next(); | |||
|
|
|||
| if let Err(err) = p.check_schedule() { | |||
| panic!("{}", err); | |||
| error!("skipping invalid schedule: {}", err); | |||
There was a problem hiding this comment.
Don't miss this bit! I haven't looked into why this is happening yet, but skipping these invalid one still results in lots of trips being made.
I suppose we need some kind of disqualifying criteria to prevent these kinds of schedules from being created upstream, but for now I just switched it to an error log.
There was a problem hiding this comment.
I'm hesitant to remove this check, because there are other places that create scenarios. And if a weird schedule gets through, I don't remember how the sim spawning layer handles it -- I think there'll be some assertion failure when the trip transition happens, which is a pretty deferred way to discover a bug.
Could we use scenario.remove_weird_schedules() instead? And maybe update the printlns there to log
There was a problem hiding this comment.
Didn't know about remove_weird_schedules - thanks. Done!
Currently this is really slow. We'll either need to speed it up
significantly or find some way to cache the scenarios and maybe
pregenerate them.
Here's a breakdown of the time spent:
- generate census scenario took 424.5807s
- building population areas for map took 2.8061s
- parsing geojson took 0.4573s
- converting to `CensusArea`s took 2.3351s
- ... plus 0.0137s
- assigning people to houses took 1.5885s
- building people took 420.1861s
It's almost entirely in the `building people` step which we can
parallelize. I'm sure there are other improvements to make after that.
activity->building map
Before:
- generate census scenario took 424.5807s
- building population areas for map took 2.8061s
- parsing geojson took 0.4573s
- converting to `CensusArea`s took 2.3351s
- ... plus 0.0137s
- assigning people to houses took 1.5885s
- building people took 420.1861s
After:
- generate census scenario took 47.6638s
- building population areas for map took 3.0067s
- parsing geojson took 0.3655s
- converting to `CensusArea`s took 2.6192s
- ... plus 0.0220s
- assigning people to houses took 1.6294s
- building people took 43.0277s
- generate census scenario took 15.4418s
- building population areas for map took 2.7773s
- parsing geojson took 0.3764s
- converting to `CensusArea`s took 2.3754s
- ... plus 0.0254s
- assigning people to houses took 1.7802s
- building people took 10.8842s
- making people in parallel (290,079)... 10.7447s
- ... plus 0.1395s
- ... plus 0.0001s
michaelkirk
force-pushed
the
mkirk/integrate-census-scenario
branch
from
e4b344c to
97d21e4
Compare
|
(just rebased atop master since #425 was merged) |
| // either pick the closest choice, or even better, randomize, but weight based on | ||
| // the cost of getting there. map.pathfind() may be helpful. | ||
|
|
||
| // For now, just pick a random one |
There was a problem hiding this comment.
behavioral change: I switched this to random rather than first.
popdat/src/lib.rs
Outdated
| .push(make_person::make_person(person, map, rng, &config)); | ||
| } | ||
|
|
||
| scenario.people.append(&mut make_person::make_people( |
There was a problem hiding this comment.
The API of append is funny, I would expect it to consume...
There was a problem hiding this comment.
This is mostly just my lack of fluency in rust.
I think append is intended for things where it makes sense to clear out but re-use the buffer like:
let buffer = vec![];
while read_into_buffer(&mut buffer) {
results.append(buffer);
}
I've switched to extend which has better semantics for this type of call.
popdat/src/make_person.rs
Outdated
| // Only consider two-way intersections, so the agent can return the same way | ||
| // they came. | ||
| // TODO: instead, if it's not a two-way border, we should find an intersection | ||
| // an incoming border "near" the outgoing border, to allow a broader set of | ||
| // realistic options. | ||
| // TODO: prefer larger thoroughfares to better reflect reality. | ||
| let commuter_borders: Vec<IntersectionID> = map | ||
| let commuter_borders: Vec<map_model::IntersectionID> = map |
There was a problem hiding this comment.
nit: IntersectionID is already imported
| // No buildings satisfy the activity. Just go somewhere off-map. | ||
| TripEndpoint::Border(*commuter_borders.choose(rng).unwrap()) | ||
| }; | ||
| let person_factory = PersonFactory::new(map); |
There was a problem hiding this comment.
Serious Java flashbacks :D
There was a problem hiding this comment.
just trying to keep the hype wave down.
| // either pick the closest choice, or even better, randomize, but weight based on | ||
| // the cost of getting there. map.pathfind() may be helpful. | ||
|
|
||
| // For now, just pick a random one |
sim/src/make/scenario.rs
Outdated
| @@ -145,7 +145,8 @@ impl Scenario { | |||
| timer.next(); | |||
|
|
|||
| if let Err(err) = p.check_schedule() { | |||
| panic!("{}", err); | |||
| error!("skipping invalid schedule: {}", err); | |||
There was a problem hiding this comment.
I'm hesitant to remove this check, because there are other places that create scenarios. And if a weird schedule gets through, I don't remember how the sim spawning layer handles it -- I think there'll be some assertion failure when the trip transition happens, which is a pretty deferred way to discover a bug.
Could we use scenario.remove_weird_schedules() instead? And maybe update the printlns there to log
|
LGTM, thanks for the PR! I'd say 440s -> 15s is quite impressive. I'm guessing the next steps are to automate the census data prep, or come up with the large-but-indexable preprocessed file. Keep me posted! |
Based on #425 so review that first.
Integrates the census based scenario and does some perf work to speed up the census scenario's creation.
Previously it'd take ~440s.
I got it down to something like 40s by caching the boundary nodes and the activity->building_ids map.
Then down to about 15s by parallelizing the make_person calls.
Oh! And some proof that it works:
