Skip to content
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

census areas import #425

Merged
merged 18 commits into from
Dec 15, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
545 changes: 298 additions & 247 deletions Cargo.lock

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,6 @@ members = [
# update dependencies often).
[profile.dev.package."*"]
opt-level = 3

[patch.crates-io]
geojson = { git = "https://github.com/georust/geojson", branch = "mkirk/try_from" }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general I think it's a little nicer to patch things here at the top level rather than updating across all the individual crates. It's maybe a little more convenient, but more importantly, it works for transitive deps too, which you can't conveniently edit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. I always forget about this trick

4 changes: 2 additions & 2 deletions fifteen_min/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ default = ["map_gui/native", "widgetry/native-backend"]

[dependencies]
abstutil = { path = "../abstutil" }
contour = { git = "https://github.com/dabreegster/contour-rs" }
geojson = "0.20.1"
contour = "0.3.0"
geojson = "0.21.0"
geom = { path = "../geom" }
log = "0.4"
map_gui = { path = "../map_gui" }
Expand Down
4 changes: 2 additions & 2 deletions game/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ built = { version = "0.4.3", optional = true, features=["chrono"] }
chrono = "0.4.15"
collisions = { path = "../collisions" }
colorous = "1.0.3"
contour = { git = "https://github.com/dabreegster/contour-rs" }
contour = "0.3.0"
downcast-rs = "1.2.0"
enumset = "1.0.1"
geojson = "0.20.1"
geojson = "0.21.0"
geom = { path = "../geom" }
instant = "0.1.7"
kml = { path = "../kml" }
Expand Down
3 changes: 2 additions & 1 deletion game/src/sandbox/gameplay/census.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ impl State<App> for CensusGenerator {
config,
&app.primary.map,
&mut app.primary.current_flags.sim_flags.make_rng(),
);
)
.expect("failed to generate scenario");
// TODO Do something with it -- save it, launch it in sandboxmode, display some
// stats about it?
return Transition::Pop;
Expand Down
2 changes: 1 addition & 1 deletion geom/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ abstutil = { path = "../abstutil" }
earcutr = { git = "https://github.com/donbright/earcutr" }
geo = "0.15.0"
geo-booleanop = "= 0.3.2"
geojson = "0.20.1"
geojson = "0.21.0"
histogram = "0.6.9"
instant = "0.1.7"
ordered-float = { version = "2.0.0", features=["serde"] }
Expand Down
40 changes: 29 additions & 11 deletions geom/src/polygon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ impl Polygon {

pub fn convex_hull(list: Vec<Polygon>) -> Polygon {
let mp: geo::MultiPolygon<f64> = list.into_iter().map(|p| to_geo(p.points())).collect();
from_geo(mp.convex_hull())
mp.convex_hull().into()
}

pub fn polylabel(&self) -> Pt2D {
Expand Down Expand Up @@ -476,19 +476,37 @@ fn to_geo(pts: &Vec<Pt2D>) -> geo::Polygon<f64> {
)
}

fn from_geo(p: geo::Polygon<f64>) -> Polygon {
Polygon::buggy_new(
p.into_inner()
.0
.into_points()
.into_iter()
.map(|pt| Pt2D::new(pt.x(), pt.y()))
.collect(),
)
impl From<geo::Polygon<f64>> for Polygon {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you ok with adding the idiomatic conversation traits? It's a little easier to discover this way since I know where to look rather than custom named methods.

I could add more for other geo types, but only added the minimum of what we needed for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these're better! I probably wrote the old thing before I understood From / Into

fn from(poly: geo::Polygon<f64>) -> Self {
let (exterior, interiors) = poly.into_inner();
Polygon::with_holes(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why I was using buggy_new.. a quick audit of convex_hull() and intersection() doesn't turn up anything that should be used in map importing, so I think this should work fine. Just never updated this call-site.

Ring::from(exterior),
interiors.into_iter().map(Ring::from).collect(),
)
}
}

impl From<Polygon> for geo::Polygon<f64> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woo, thanks for fixing this! When I originally wrote this, I didn't know about earcutr for triangulating holes, and I think I confused multipolygons / polygons with inner portions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated these From implementations now that I fully understand what ABStreet geom::Polygon's expect.

fn from(poly: Polygon) -> Self {
if let Some(mut rings) = poly.rings {
let exterior = rings.pop().expect("expected poly.rings[0] to be exterior");
let interiors: Vec<geo::LineString<f64>> =
rings.into_iter().map(geo::LineString::from).collect();
Self::new(exterior.into(), interiors)
} else {
let exterior_coords = poly
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this will break for a polygon produced by polyline.make_polygons, for instance, because points won't form a ring. It looks like on the geo side, it'll pts.push(pts[0]) on input if needed. That'll still turn out to be nonsense for this, since the points will go back and forth.

I think this is fine for now. Ideally every geom::Polygon will properly have an exterior ring. aka, we have to whittle down the callers of Polygon::buggy_new and Polygon::precomputed. Some of those will be hard, maybe impossible (like triangulated polygons coming from lyon).

.points
.into_iter()
.map(geo::Coordinate::from)
.collect::<Vec<_>>();
let exterior = geo::LineString(exterior_coords);
Self::new(exterior, Vec::new())
}
}
}

fn from_multi(multi: geo::MultiPolygon<f64>) -> Vec<Polygon> {
multi.into_iter().map(from_geo).collect()
multi.into_iter().map(Polygon::from).collect()
}

fn downsize(input: Vec<usize>) -> Vec<u16> {
Expand Down
27 changes: 27 additions & 0 deletions geom/src/pt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,3 +182,30 @@ impl HashablePt2D {
Pt2D::new(self.x_nan.into_inner(), self.y_nan.into_inner())
}
}

impl From<Pt2D> for geo::Coordinate<f64> {
fn from(pt: Pt2D) -> Self {
geo::Coordinate {
x: pt.inner_x,
y: pt.inner_y,
}
}
}

impl From<Pt2D> for geo::Point<f64> {
fn from(pt: Pt2D) -> Self {
geo::Point::new(pt.inner_x, pt.inner_y)
}
}

impl From<geo::Coordinate<f64>> for Pt2D {
fn from(coord: geo::Coordinate<f64>) -> Self {
Pt2D::new(coord.x, coord.y)
}
}

impl From<geo::Point<f64>> for Pt2D {
fn from(point: geo::Point<f64>) -> Self {
Pt2D::new(point.x(), point.y())
}
}
17 changes: 17 additions & 0 deletions geom/src/ring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,20 @@ impl fmt::Display for Ring {
write!(f, "])")
}
}

impl From<Ring> for geo::LineString<f64> {
fn from(ring: Ring) -> Self {
let coords = ring
.pts
.into_iter()
.map(geo::Coordinate::from)
.collect::<Vec<_>>();
Self(coords)
}
}

impl From<geo::LineString<f64>> for Ring {
fn from(line_string: geo::LineString<f64>) -> Self {
Self::must_new(line_string.0.into_iter().map(Pt2D::from).collect())
}
}
2 changes: 1 addition & 1 deletion headless/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ edition = "2018"

[dependencies]
abstutil = { path = "../abstutil" }
geojson = "0.20.1"
geojson = "0.21.0"
geom = { path = "../geom" }
hyper = "0.13.9"
lazy_static = "1.4.0"
Expand Down
2 changes: 1 addition & 1 deletion importer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ abstutil = { path = "../abstutil" }
collisions = { path = "../collisions" }
convert_osm = { path = "../convert_osm" }
csv = "1.1.4"
geojson = "0.20.1"
geojson = "0.21.0"
geom = { path = "../geom" }
gdal = { version = "0.6.0", optional = true }
kml = { path = "../kml" }
Expand Down
4 changes: 2 additions & 2 deletions map_gui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ release_s3 = []
aabb-quadtree = "0.1.0"
abstutil = { path = "../abstutil" }
colorous = "1.0.3"
contour = { git = "https://github.com/dabreegster/contour-rs" }
contour = "0.3.0"
flate2 = "1.0.19"
futures = { version = "0.3.8", optional = true }
futures-channel = { version = "0.3.8", optional = true }
geojson = "0.20.1"
geojson = "0.21.0"
geom = { path = "../geom" }
instant = "0.1.7"
js-sys = { version = "0.3.45", optional = true }
Expand Down
4 changes: 4 additions & 0 deletions popdat/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@ edition = "2018"

[dependencies]
abstutil = { path = "../abstutil" }
geo = "0.16.0"
geojson = { version = "0.21.0", features = ["geo-types"] }
geom = { path = "../geom" }
log = "0.4.11"
map_model = { path = "../map_model" }
rand = "0.7.0"
rand_xorshift = "0.2.0"
sim = { path = "../sim" }
serde_json = "1.0.60"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sort. I really wish cargo fmt would get support for this :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why this is so hard for me. Thanks for your diligence!

anyhow = "1.0.35"
2 changes: 1 addition & 1 deletion popdat/src/distribute_people.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ pub fn assign_people_to_houses(
let pct_overlap = Polygon::union_all(area.polygon.intersection(map.get_boundary_polygon()))
.area()
/ area.polygon.area();
let num_residents = (pct_overlap * (area.total_population as f64)) as usize;
let num_residents = (pct_overlap * (area.population as f64)) as usize;
debug!(
"Distributing {} residents to {} buildings. {}% of this area overlapped with the map, \
scaled residents accordingly.",
Expand Down
127 changes: 121 additions & 6 deletions popdat/src/import_census.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,126 @@
use map_model::Map;

use crate::CensusArea;
use abstutil::Timer;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: For consistency, we should still split the imports based on category... std, external crates, other crates in this workspace, the current crate. I guess we don't have any contributors using an IDE that forces this order, so not a huge deal either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to abide

use geo::algorithm::intersects::Intersects;
use geojson::GeoJson;
use map_model::Map;
use std::convert::TryFrom;

impl CensusArea {
pub fn find_data_for_map(_map: &Map) -> Result<Vec<CensusArea>, String> {
// TODO importer/src/utils.rs has a download() helper that we could copy here. (And later
// dedupe, after deciding how this crate will integrate with the importer)
todo!()
pub fn find_data_for_map(map: &Map, timer: &mut Timer) -> anyhow::Result<Vec<CensusArea>> {
// TODO eventually it'd be nice to lazily download the info needed. For now we expect a
// prepared geojson file to exist in data/system/<city>/population_areas.geojson
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

system makes sense to me, because we'll need this at runtime. (Just thinking aloud)

//
// expected geojson formatted contents like:
// {
// "type": "FeatureCollection",
// "features": [
// {
// "type": "Feature",
// "properties": { "population": 123 },
// "geometry": {
// "type": "MultiPolygon",
// "coordinates": [ [ [ [ -73.7, 40.8 ], [ -73.7, 40.8 ], ...] ] ] ]
// }
// },
// {
// "type": "Feature",
// "properties": { "population": 249 },
// "geometry": {
// "type": "MultiPolygon",
// "coordinates": [ [ [ [ -73.8, 40.8 ], [ -73.8, 40.8 ], ...] ] ]
// }
// },
// ...
// ]
// }
//
// Note: intuitively you might expect a collection of Polygon's rather than MultiPolygons,
// but anecdotally, the census data I've seen uses MultiPolygons. In practice almost
// all are MultiPoly's with just one element, but some truly have multiple polygons.
//
// When we implement downloading, importer/src/utils.rs has a download() helper that we
// could copy here. (And later dedupe, after deciding how this crate will integrate with
// the importer)
let path = abstutil::path(format!(
"system/{}/population_areas.geojson",
map.get_name().city
));
let bytes = abstutil::slurp_file(&path).map_err(|s| anyhow!(s))?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I guess String doesn't implement std::error::Error, so we need the conversion here. I guess a good refactor would change all of the result types in all abst code to use anyhow::Result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep exactly.

debug!("parsing geojson at path: {}", &path);
timer.start("parsing geojson");
let geojson = GeoJson::from_reader(&*bytes)?;
timer.stop("parsing geojson");
let mut results = vec![];
let collection = geojson::FeatureCollection::try_from(geojson)?;

let map_area = map.get_boundary_polygon();
let bounds = map.get_gps_bounds();

use geo::algorithm::map_coords::MapCoordsInplace;
let mut geo_map_area: geo::Polygon<_> = map_area.clone().into();
geo_map_area.map_coords_inplace(|c| {
let projected = geom::Pt2D::new(c.0, c.1).to_gps(bounds);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to get the map poly in wgs84?

Though maybe it's irrelevant, because the other question I need to answer is: is there a blessed way to convert a poly from wgs84 to map coords?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPSBounds::convert? Or if you want to enforce all points are in bounds, try_convert?

(projected.x(), projected.y())
});

debug!("collection.features: {}", &collection.features.len());
timer.start("converting to `CensusArea`s");
for feature in collection.features.into_iter() {
let population = match feature
.property("population")
.ok_or(anyhow!("missing 'population' property"))?
{
serde_json::Value::Number(n) => n
.as_u64()
.ok_or(anyhow!("unexpected population number: {:?}", n))?
as usize,
_ => {
bail!(
"unexpected format for 'population': {:?}",
feature.property("population")
);
}
};

let geometry = feature
.geometry
.ok_or(anyhow!("geojson feature missing geometry"))?;
let mut multi_poly = geo::MultiPolygon::<f64>::try_from(geometry.value)?;
let mut geo_polygon = multi_poly
.0
.pop()
.ok_or(anyhow!("multipolygon was unexpectedly empty"))?;
if !multi_poly.0.is_empty() {
// I haven't looked into why this is happening - but intuitively a census area could
// include separate polygons - e.g. across bodies of water. In practice they are a
// vast minority, so we naively just take the first one for now.
warn!(
"dropping {} polygons from census area with multiple polygons",
multi_poly.0.len()
);
}

if !geo_polygon.intersects(&geo_map_area) {
debug!(
"skipping polygon outside of map area. polygon: {:?}, map_area: {:?}",
geo_polygon, geo_map_area
);
continue;
}

geo_polygon.map_coords_inplace(|(x, y)| {
let point = geom::LonLat::new(*x, *y).to_pt(bounds);
(point.x(), point.y())
});
let polygon = geom::Polygon::from(geo_polygon);
results.push(CensusArea {
polygon,
population,
});
}
debug!("built {} CensusAreas within map bounds", results.len());
timer.stop("converting to `CensusArea`s");

Ok(results)
}
}
16 changes: 14 additions & 2 deletions popdat/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,12 @@
#[macro_use]
extern crate log;

#[macro_use]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm nitpicking style, maybe we should sort this above the log too. Although honestly if rustfmt doesn't do it, stuff like this is just busywork. :\

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it that you'd prefer to keep the alphabetical within the sections? That sounds fine - just making sure I understand.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that's the general idea. With macro_use I guess it's not a hard-and-fast rule, because I think the order of these can have a real effect if there are interdependencies.

extern crate anyhow;

use rand_xorshift::XorShiftRng;

use abstutil::Timer;
use geom::Polygon;
use geom::{Distance, Time};
use map_model::{BuildingID, Map};
Expand All @@ -38,7 +42,7 @@ mod make_person;
/// have two overlapping areas.
pub struct CensusArea {
pub polygon: Polygon,
pub total_population: usize,
pub population: usize,
// TODO Not sure what goes here, whatever census data actually has that could be useful
}

Expand Down Expand Up @@ -105,16 +109,24 @@ pub fn generate_scenario(
) -> Result<Scenario, String> {
// find_data_for_map may return an error. If so, just plumb it back to the caller using the ?
// operator
let areas = CensusArea::find_data_for_map(map)?;
let mut timer = Timer::new("generate census scenario");
timer.start("building population areas for map");
let areas = CensusArea::find_data_for_map(map, &mut timer).map_err(|e| e.to_string())?;
timer.stop("building population areas for map");

timer.start("assigning people to houses");
let people = distribute_people::assign_people_to_houses(areas, map, rng, &config);
timer.stop("assigning people to houses");

let mut scenario = Scenario::empty(map, scenario_name);
timer.start("building people");
for person in people {
// TODO If we need to parallelize because make_person is slow, the sim crate has a fork_rng
// method that could be useful
scenario
.people
.push(make_person::make_person(person, map, rng, &config));
}
timer.stop("building people");
Ok(scenario)
}