coderholic / django-cities

Countries and cities of the world for Django projects
MIT License
921 stars 373 forks source link

Latitude and Longitude Switched in Import Script #3

Closed trendymoniker closed 12 years ago

trendymoniker commented 13 years ago

Hi Ben, Thanks for this project -- the import script especially is very useful!

I think I've spotted one small bug: the import script seems to be switching latitude and longitude for both City and District locations.

Lines 86 and 145 of import.py construct City and District Point objects as: Point(float(items[4]), float(items[5]), where I think it should be Point(float(items[5]), float(items[4]). In GeoNames cities1000.txt, col 5 (Items[4]) is Latitude and col 6 (items[5]) is Longitude. The Point constructor takes the arguments Point(x, y) or Point(Lon, Lat) instead of point Point(Lat, Lon) (see http://groups.google.com/group/django-users/browse_thread/thread/80dae95ee8f76361?pli=1).

Thanks again and keep up the good work!

Kevin

coderholic commented 13 years ago

Hi Kevin,

Thanks for reporting the bug. I've looked into it and you're absolutely right. I never picked up on the bug because I always thought that the Point constructor took Point(Lat, Lon), so all of my code assumes that Point.x is lat and Point.y is lon. I think the only side effect of this will be slightly incorrect distance calculations. It's definitely something I should fix though!

Cheers, Ben

georgema1982 commented 13 years ago

Hi Ben,

Does that mean that the initial data all have wrong locations? I'm asking as I have a django app depending on your app.

coderholic commented 13 years ago

It means the latitude is stored in the x field, and the longitude in the y field.

georgema1982 commented 13 years ago

Hi Ben, then it's upside down. For those who want to display the location on the map, it's a trouble. I guess the quick fix for now is scan the initial data and change the positions of all the point values.

coderholic commented 13 years ago

It's not a problem for those displaying it on a map. You just have to remember that x is latitude and y is longitude. It's only a problem if you want to calculate the distance between 2 points, because the calculation changes based on latitude, not longitude.

georgema1982 commented 13 years ago

Hi Ben, it's definitely not a problem for displaying but a trouble. Because in a location based website, there might be other tables containing user's address locations as well. There will be an inconsistency between geo-cities way of storing point and other tables' way. As time passes by, it might cause a mess-up. I'm thinking about creating a command to fix the positions if users have already imported the initial data. I will fork your project and get back to you once I have time to finish it.

coderholic commented 13 years ago

Yes, it's definitely something I'll fix. Something you can do for now is add a latitude and longitude property on City that return x and y.

georgema1982 commented 13 years ago

Hi Ben, I've fixed the location errors in my fork and add another admin command to only import selected countries. But now I encounter server error all the time when sending pull request. Still waiting for github's support.

georgema1982 commented 13 years ago

OK, got github support's reponse. It's because the dump file is too long on one single line. They are fixing this issue but at the meantime, they suggest you pull my branch manually.

coderholic commented 13 years ago

OK no problem. Can you send me the pull instructions?

coderholic commented 13 years ago

...or just a link to the diff.

georgema1982 commented 13 years ago

Hi Ben,

Here's my fork: https://github.com/georgema1982/django-cities

Following files need to be pulled manually:

README: Explain what the new command does

models.py: Switch longitude and latitude to ensure distance is correct

geonames_dump.json: The new dump with location errors fixed. If the user has already imported the dump with wrong locations, simply import this new dump which will correct the wrong locations.

initial_data.json: It's removed to prevent the fixture imported every time sycdb is run.

load_geonames.py: The new command able to selectively import countries. If the user has already imported the dump with wrong locations, the selected countries' locations will be fixed once it's run.

georgema1982 commented 13 years ago

Hi Ben,

Could you take a look at files mentioned above and decide if it's worth manual merging? As github still can't handle extremely long file, I'm not able to give a diff link. Every attempt to see a diff give me an error.

coderholic commented 13 years ago

Hi George,

I've had a look and I'd definitely like to merge them. Any idea on how long it'll be before github fix the issue though? If it's going to be a while I'll look into manually merging the changes.

Cheers, Ben

georgema1982 commented 13 years ago

Hi Ben,

The github guy didn't say the timeline or if such timeline exists. The only bad boy is the dump file. I guess if I prettify it, github will like it more. But that will increase its size though. Would you like me to prettify it for the sake of ease of merging or you'd like to do some manual work?

coderholic commented 13 years ago

It'd be great if you could prettify it to make it easier to merge the changes.

Thanks, Ben