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

Join T intersections (was: Ensure nodes are shared correctly) #8

Closed
lxbarth opened this Issue Sep 10, 2013 · 15 comments

Comments

Projects
None yet
5 participants
@lxbarth
Member

lxbarth commented Sep 10, 2013

From https://lists.openstreetmap.org/pipermail/imports/2013-September/002154.html

The sample OSM file is not clean in the JOSM validator. The row houses are not being merged correctly, they are missing shared nodes.

Sounds a bit like osmlab/dcbuildings#11

@lxbarth

This comment has been minimized.

Show comment
Hide comment
@lxbarth

lxbarth Sep 25, 2013

Member

I'm not sure this report is correct at all, based on https://github.com/osmlab/nycbuildings/blob/master/convert.py#L68 there should be proper shared nodes.

Needs a double check.

Member

lxbarth commented Sep 25, 2013

I'm not sure this report is correct at all, based on https://github.com/osmlab/nycbuildings/blob/master/convert.py#L68 there should be proper shared nodes.

Needs a double check.

@lxbarth

This comment has been minimized.

Show comment
Hide comment
@lxbarth

lxbarth Oct 3, 2013

Member

So here's what's going on. This is non-critical as it can be fixed manually in JOSM before upload but it does take time to resolve these issues by hand. I'll look into solving this on the conversion level.

Member

lxbarth commented Oct 3, 2013

So here's what's going on. This is non-critical as it can be fixed manually in JOSM before upload but it does take time to resolve these issues by hand. I'll look into solving this on the conversion level.

@iandees

This comment has been minimized.

Show comment
Hide comment
@iandees

iandees Oct 3, 2013

Member

Yes, that node should be joined with the way underneath it.

Member

iandees commented Oct 3, 2013

Yes, that node should be joined with the way underneath it.

@lxbarth

This comment has been minimized.

Show comment
Hide comment
@lxbarth

lxbarth Oct 5, 2013

Member

This can be fixed very easily in JOSM:

Needs documentation in tasking manager. I don't think we should fix this on the conversion script level for now, as this would cost a lot of work.

Member

lxbarth commented Oct 5, 2013

This can be fixed very easily in JOSM:

Needs documentation in tasking manager. I don't think we should fix this on the conversion script level for now, as this would cost a lot of work.

@emacsen

This comment has been minimized.

Show comment
Hide comment
@emacsen

emacsen Oct 5, 2013

You're right that this is a /hard/ problem. At the same time, we have a lot that we're asking users to do. Perhaps there's something we can do to find this out either after conversion but before upioad to task manger, or (worst case) finding these problems programatically in OSM to fix later? (this is not what I'd prefer, though)

emacsen commented Oct 5, 2013

You're right that this is a /hard/ problem. At the same time, we have a lot that we're asking users to do. Perhaps there's something we can do to find this out either after conversion but before upioad to task manger, or (worst case) finding these problems programatically in OSM to fix later? (this is not what I'd prefer, though)

@lxbarth

This comment has been minimized.

Show comment
Hide comment
@lxbarth

lxbarth Oct 5, 2013

Member

Fixing in JOSM is doable for now, I want to see how this approach jives on the Saturday event then adjust where necessary.

Member

lxbarth commented Oct 5, 2013

Fixing in JOSM is doable for now, I want to see how this approach jives on the Saturday event then adjust where necessary.

@MateoV

This comment has been minimized.

Show comment
Hide comment
@MateoV

MateoV Oct 17, 2013

Contributor

Essentially fixed in the above commit. This is happening iteratively when adding a building way. First it uses rtree to check for other intersecting buildings, then checks if the other buildings' nodes fall on (or extremely close to) the edges as the way is being built into osm xml.

Note that this increases the processing time by about 10x (taking NYC from 1 hour -> 10 hours), which while not ideal is acceptable in my opinion as this theoretically only needs to run once.

cc @sgillies

Contributor

MateoV commented Oct 17, 2013

Essentially fixed in the above commit. This is happening iteratively when adding a building way. First it uses rtree to check for other intersecting buildings, then checks if the other buildings' nodes fall on (or extremely close to) the edges as the way is being built into osm xml.

Note that this increases the processing time by about 10x (taking NYC from 1 hour -> 10 hours), which while not ideal is acceptable in my opinion as this theoretically only needs to run once.

cc @sgillies

@emacsen

This comment has been minimized.

Show comment
Hide comment
@emacsen

emacsen Oct 17, 2013

Matt, I know 10 hours sounds like a lot, but I think this import will take between 1000 and 2000 man hours, and then the review.

Considering that I spent far more than 10 hours just cleaning 30% of the data from the weekend of imports- this is pretty small. Of course your time is worth far more, but at least this code should be somewhat applicable to future imports.

emacsen commented Oct 17, 2013

Matt, I know 10 hours sounds like a lot, but I think this import will take between 1000 and 2000 man hours, and then the review.

Considering that I spent far more than 10 hours just cleaning 30% of the data from the weekend of imports- this is pretty small. Of course your time is worth far more, but at least this code should be somewhat applicable to future imports.

MateoV added a commit that referenced this issue Oct 18, 2013

Merge pull request #20 from osmlab/node-sharing
share nodes between intersecting ways, ref #8
@sgillies

This comment has been minimized.

Show comment
Hide comment
@sgillies

sgillies Oct 18, 2013

@MateoV thanks for the reminder to share the links to the PostGIS topology stuff:

http://postgis.org/documentation/manual-2.0/Topology.html
http://strk.keybit.net/blog/2013/03/08/on-the-fly-simplification-of-topologically-defined-geometries/

I'd love to look into making a Python iterface to PostGIS' TopoGeometry. Things in PostGIS tend to be a bit wound up with PostgreSQL, so it might not work out. Worth a look down the road, I think.

sgillies commented Oct 18, 2013

@MateoV thanks for the reminder to share the links to the PostGIS topology stuff:

http://postgis.org/documentation/manual-2.0/Topology.html
http://strk.keybit.net/blog/2013/03/08/on-the-fly-simplification-of-topologically-defined-geometries/

I'd love to look into making a Python iterface to PostGIS' TopoGeometry. Things in PostGIS tend to be a bit wound up with PostgreSQL, so it might not work out. Worth a look down the road, I think.

@lxbarth

This comment has been minimized.

Show comment
Hide comment
@lxbarth

lxbarth Oct 21, 2013

Member

@MateoV - I know you did a reexport here, but did you also push to s3?

I just found an osm file with non-merged T-intersects http://127.0.0.1:8111/import?url=https://s3.amazonaws.com/osm_us_backup/imports/nyc/buildings-addresses-64051.osm

Member

lxbarth commented Oct 21, 2013

@MateoV - I know you did a reexport here, but did you also push to s3?

I just found an osm file with non-merged T-intersects http://127.0.0.1:8111/import?url=https://s3.amazonaws.com/osm_us_backup/imports/nyc/buildings-addresses-64051.osm

@MateoV

This comment has been minimized.

Show comment
Hide comment
@MateoV

MateoV Oct 21, 2013

Contributor

@lxbarth Yes I pushed to s3. There must still be some edge cases.

Contributor

MateoV commented Oct 21, 2013

@lxbarth Yes I pushed to s3. There must still be some edge cases.

@lxbarth

This comment has been minimized.

Show comment
Hide comment
@lxbarth

lxbarth Oct 21, 2013

Member

@MateoV - can you check what's been going on in this case? Are the unjoined T-intersects on 64051 outside of the buffer you're applying?

Member

lxbarth commented Oct 21, 2013

@MateoV - can you check what's been going on in this case? Are the unjoined T-intersects on 64051 outside of the buffer you're applying?

@MateoV

This comment has been minimized.

Show comment
Hide comment
@MateoV

MateoV Oct 21, 2013

Contributor

@lxbarth Looking into. Just crosschecked with the original file (pre-fix). It has 32 errors.

Contributor

MateoV commented Oct 21, 2013

@lxbarth Looking into. Just crosschecked with the original file (pre-fix). It has 32 errors.

@MateoV

This comment has been minimized.

Show comment
Hide comment
@MateoV

MateoV Oct 21, 2013

Contributor

Also I have stats from the last export. 5.5 hours, which is less than expected.

Contributor

MateoV commented Oct 21, 2013

Also I have stats from the last export. 5.5 hours, which is less than expected.

@lxbarth

This comment has been minimized.

Show comment
Hide comment
@lxbarth

lxbarth Oct 22, 2013

Member

This is fixed and rolled out on the tasking manager now.

Member

lxbarth commented Oct 22, 2013

This is fixed and rolled out on the tasking manager now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment