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

Potentially wrong results returned #34

Open
jannikmi opened this issue Apr 13, 2016 · 17 comments
Open

Potentially wrong results returned #34

jannikmi opened this issue Apr 13, 2016 · 17 comments
Assignees
Labels

Comments

@jannikmi
Copy link

I wrote a package using the same data and compared my results to the results of this package for aprox. 5M points. Those are a few of the points where I got mismatches:

(lng, lat), result timezonefinder, result pytzwhere
(-110.58195555806134, 35.53178795327783), 'America/Phoenix', 'America/Denver']
(-110.41118747093144, 35.76969036563554), 'America/Phoenix', 'America/Denver']
(-110.69776990691068, 35.59283347945522), 'America/Phoenix', 'America/Denver']
(-110.91327628315915, 36.11131540134208), 'America/Phoenix', 'America/Denver']
(-110.26814562443687, 35.98507252905026), 'America/Phoenix', 'America/Denver']
(-133.73396065378114, 68.38068073677294), 'America/Inuvik', 'America/Yellowknife']

These might be errors in my algorithm, but I still though you might want to check this out.

This is my package:
https://github.com/MrMinimal64/timezonefinder

@cstich
Copy link
Contributor

cstich commented Apr 22, 2016

Thanks for the info. As far as I remember that area of Arizona is a mess due to the Navajo Nation and the Hopi. I will check it out though :)

@cstich cstich added the bug label Apr 22, 2016
@cstich
Copy link
Contributor

cstich commented Apr 22, 2016

I checked the points manually. It looks like pytzwhere is not dealing with polygons within polygons correctly. Your results are indeed correct.

@jannikmi
Copy link
Author

I also think it is because of polygons within polygons. My algorithms is stopping when the point was found to be included in a polygon, too. I decided not to change my algorithm, because actually this is a mistake within the data and correcting it would make it ugly and slow.

@cstich
Copy link
Contributor

cstich commented Apr 22, 2016

That makes sense. However, it is actually not a mistake in the data. It is legitimate for timezones to have timezones within them.

@jannikmi
Copy link
Author

Why? Then your data would suggest that this area is in two timezones... I don't think that makes sense.

@cstich
Copy link
Contributor

cstich commented Apr 22, 2016

I should rephrase this for clarity. I didn't mean to say that a point would actually be in two polygons, but within the American/Denver timezone there is a part that is American/Phoenix. This video does a much better job at explaining it than I ever could.

@jannikmi
Copy link
Author

But still a timezone has to be unique and therefore the surrounding timezone would need to be excluded from the surrounded tz. Otherwise algorithms don't really have a chance of deciding which timezone it is (except hardcoding it) or am I wrong here?

@pegler
Copy link
Owner

pegler commented Apr 22, 2016

Polygons can't have holes in them. To ensure every coordinate was located within exactly one polygon would require changing the data to have a "seam" connecting the hole with the outer boundary. So it's just a matter of convention. We pull the data from http://efele.net/maps/tz/world/ and they follow one convention. The only solution here would be to reprocess the data to remove all layered polygons.

@cstich
Copy link
Contributor

cstich commented Apr 22, 2016

The original data from Eric Muller does have holes though. I just checked.

@pegler
Copy link
Owner

pegler commented Apr 22, 2016

"Holes" as in the polygon defines both an outer and inner boundary?

@jannikmi
Copy link
Author

For me the ideal solution would be to convince them to correct the data (for me layered polygons are mistakes), but I don't know realistic this is.
By the way do you have a way of automatically creating your .csv from efele.net data?
I thought about that issue , but I just decided to leave it that way for now. There are just a few points where this matters and the effort for fixing this seemed to big.

@cstich
Copy link
Contributor

cstich commented Apr 22, 2016

Yes a "hole" as in having an outer and inner boundary

@pegler
Copy link
Owner

pegler commented Apr 22, 2016

I remember running into issues with "holes" when initially working on this project. I can dig in next week to see if I can figure it out

@cstich
Copy link
Contributor

cstich commented Apr 22, 2016

@MrMinimal64 I wrote a very short shell script that converts Eric Muller's shapefiles into a GEOjson.

@pegler Maybe it makes sense to use the multi-polygon timezone data? I used the polygon one so far but this discussion makes me wonder if we shouldn't use the other.

@cstich cstich self-assigned this Apr 24, 2016
@cstich
Copy link
Contributor

cstich commented Apr 24, 2016

I think I know how to fix this. I am working on it.

@cstich
Copy link
Contributor

cstich commented May 16, 2016

This should now be fixed in the development branch. The development branch now correctly deals with holes. There are a few other little things I would like to add to that branch before we can release it but for testing timezonefinder it should already be sufficient

@famanson
Copy link
Contributor

@cstich @pegler hi guys, is there any new progress on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants