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

propagate translator exceptions (see for_pnorman branch) #21

Merged
merged 1 commit into from
Jun 29, 2013

Conversation

bcavagnolo
Copy link

Hello Paul,

I've been working on a translator and found that exceptions from my translator functions were not appearing. I came up with a solution and would be grateful if you'd except it into the mainline. I also fixed a failing test. It seems that the sp_usinas test data attributes are in Latin-1 not UTF-8. But if the test passes on your machine, maybe I'm wrong about this.

Ciao,
Brian

@pnorman
Copy link
Owner

pnorman commented Apr 1, 2013

I'll be merging e6fc0b0 but not 6a494d3, but first I need to dig through my git cheat sheets to check how.

I want to implement #22 short term and medium term figure out why the utf8 test passes on my machine but not yours. What does ogrinfo --version report?

@bcavagnolo
Copy link
Author

$ ogrinfo --version
GDAL 1.7.3, released 2010/11/10

This is what ships in Ubuntu 12.04. BTW, I guessed that you were using a newer version of libgdal. I also get a slight difference in the srs output for the utf8 test:

  •      SPHEROID["GRS_1967_Modified",6378160.0,298.25]],
    
  •      SPHEROID["GRS_1967_Truncated",6378160.0,298.25]],
    

There seems to be some activity in gdal post-1.7 that may account for this difference:
http://trac.osgeo.org/gdal/ticket/4345

Perhaps it also accounts for the utf8 failure.

Ciao,
Brian

While there is only one example of replacejwithi being called, it
is clear that the arguments are meant to be type Geometry, not
(Geometry, role) tuple.  So when replacing j with i, preserve the
existing role.
@bcavagnolo bcavagnolo closed this Apr 4, 2013
@bcavagnolo bcavagnolo reopened this Apr 4, 2013
@bcavagnolo
Copy link
Author

Hey Paul,

I see that you merged my patch. I added another fix to this for_pnorman branch for replacing children of relations. I also eliminated the utf8 failure patch. Can you pull this new patch? Sorry if I'm veering from github protocol here.

Ciao,
Brian

@pnorman
Copy link
Owner

pnorman commented Apr 4, 2013

replaceiwithj runs in a very tight loop, I'll have to profile these changes to check if they negatively impact timing

@bcavagnolo
Copy link
Author

Understood. Do you know of any translators or other code that are currently using the Relation object's replaceiwithj? I don't see one in the existing code. I figured the common case might be that two multipolygon relations have the same (untagged) Way. And this is the case that I was fixing when I discovered this issue.

@pnorman
Copy link
Owner

pnorman commented Apr 7, 2013

Thinking about it, I'm not sure if the relation replacejwithi gets called at all. It will when shared ways on areas gets added as an option.

A translation shouldn't be calling these, they're used deep in the OCG -> OSM data model conversion

Do you have a testcase for where the relation replacejwithi is generating an error?

The current version of that line came about in a086c32 but it was simply a mirror of the changes to the way replacejwithi. Because the logic is from @andrewguertin I need to work fairly hard to trace it through. It could even be from @IvanSanchez's earlier (SVN) version.

@bcavagnolo
Copy link
Author

Hey Paul,

This Relation.replacejwithi issue came up while I was working to eliminate multiple multipolygons from a parcel shapefile. The code I'm using is in preOutputTransform function in https://github.com/ual/parceltools/blob/master/translations/sf.py. It is inspired from the code in ogr2osm that replaces duplicate points. The test file I was using is: https://github.com/ual/parceltools/blob/master/tests/multiparcel.shp and friends. There's a multiparcel-ref.osm file that is the target output.

Perhaps a translator should not be invoking replacejwithi. But the translator should probably be invoked when tags on duplicated geometries need to be reconciled, and the detect/replace code should probably be in ogr2osm. This was my first approach. But a lot of questions came up about corner cases. Like what if you have two geometrically identical ways, one wrapped up in a Feature with attributes, and the other that is just a multipolygon Relation's child? My head started spinning on these, so I just focused on getting my specific case working in my specific translator code instead of solving the problem in general. I'm happy to work this up as a contribution to ogr2osm if you think this is worthy.

Ciao,
Brian

@pnorman pnorman merged commit 0c1716d into pnorman:master Jun 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants