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

Initial commit of generating random data #60

Merged
merged 9 commits into from
Aug 11, 2015
Merged

Initial commit of generating random data #60

merged 9 commits into from
Aug 11, 2015

Conversation

rowanwins
Copy link
Contributor

Hi @frewsxcv

As suggested here is a pull request for work initial work on generating random data.

Cheers
Rowan

@frewsxcv
Copy link
Collaborator

If you want the commit to be tied to your GitHub username, you should follow these instructions:

https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/

Not mandatory, but might be nice to give you proper credit. Once you do that, you can just git commit --amend and git push -f to repush the commit.

@frewsxcv
Copy link
Collaborator

frewsxcv commented Jul 2, 2015

Also, if you have time, there are style/linting issues which you can see here:

https://travis-ci.org/frewsxcv/python-geojson/jobs/69054890

If you don't get around to them, I can fix them myself later

@rowanwins
Copy link
Contributor Author

Hi @frewsxcv
Thanks, I didn't realise you could actually see what was failing, now that you point it out its obvious! And I just installed a python linter so that helped :)
I've still got one issue to do with line length given all my params, im not sure about the best way to handle that.

Anyway let me know how you're getting on, no rush of course
Cheers
Rowan

@frewsxcv
Copy link
Collaborator

frewsxcv commented Jul 3, 2015

Before:

 def generate_random(featureType, numberFeatures=1, numberVertices=3, boundingBox=[-180.0, -90.0, 180.0, 90.0]): 

After:

 +def generate_random(featureType, numberFeatures=1, numberVertices=3,
                      boundingBox=[-180.0, -90.0, 180.0, 90.0]): 

@frewsxcv
Copy link
Collaborator

frewsxcv commented Jul 3, 2015

More info about indentation from the official style guide

http://legacy.python.org/dev/peps/pep-0008/#indentation

@rowanwins
Copy link
Contributor Author

Hi @frewsxcv

Sorry for the delay in progress, just a quick update, I've been a bit stuck on the random polygon generation, my original idea of simply sorting the coords failed (given hindsight that was kind of obvious!). The geojson-random library from mapbox contains code to generate random polys although Im having a hard time understanding the maths, I think I need to sit down with a piece of paper to fully understand whats going on.

Cheers
Rowan

@frewsxcv
Copy link
Collaborator

If you want, we can merge in a basic implementation of the random geometry algorithm you have now, then when you understand the math more, open a new PR improving it

@rowanwins
Copy link
Contributor Author

Hi @frewsxcv

well I think I've pretty got things working now at a basic level, hooray!

For the polygon creation I took some code from here although this code wasn't designed for dealing with lat lons so its not working with the bounding box properly yet, I still need to finalise the maths around it but it's at least working for the timebeing.

This linting business took a while I get my head around, I've think I've now got a decent linter installed so fingers crossed things are looking better now!

Anyway let me know what you think, I hope it forms a useful part of the library.

Cheers
Rowan

@frewsxcv
Copy link
Collaborator

frewsxcv commented Aug 2, 2015

I haven't forgotten about this! I'll be returning home from my month long traveling in a couple days. Hopefully I'll look at this later this week

a linestring or polygon will have
:type int: defaults to 3
:param boundingBox: A bounding box in which features will be restricted to
:type string: defaults to the world - [-180.0, -90.0, 180.0, 90.0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be :type list:?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure @frewsxcv :)
are you referring to a particular line?
Your comment is a bit cryptic to this novice python-er

Copy link
Collaborator

Choose a reason for hiding this comment

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

With GitHub Pull Request, you can make comments on specific lines. The comment I made above is referring to line number 74. You can see this here. Let me know if you don't understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh for some reason lines 59-74 came through with the comment which is why I was confused.

Re your question I'll take it your suggesting that it should be a list so i've committed again :)

@frewsxcv
Copy link
Collaborator

frewsxcv commented Aug 5, 2015

@rowanwins Looks good! You're welcome to add yourself under the Credits in the README.md if you want before I merge this

@rowanwins
Copy link
Contributor Author

Hi @frewsxcv , happy not to be credited

I do have one final question which it would be good to get your opinion on. I've been debating removing the multi-feature support. The current approach locks people into a geometrycollection and doest allow properties. It may be easier to let people write the loop themselves if they want multi-features. What do you think?

FYI I I just used this to generate 100,000 random points with properties in a feature collection using this approach, took ~5s to run. Very handy for test data!

@frewsxcv
Copy link
Collaborator

frewsxcv commented Aug 6, 2015

I think it's okay to leave out the multi-feature support; writing a loop should be simple enough, though it's up to you whether you want to keep it or not

FYI I I just used this to generate 100,000 random points with properties in a feature collection using this approach, took ~5s to run.

:D

@rowanwins
Copy link
Contributor Author

@frewsxcv have removed the multi-feature support and re-committed, this is it I reckon...

@frewsxcv
Copy link
Collaborator

Looks great @rowanwins, thanks!

frewsxcv added a commit that referenced this pull request Aug 11, 2015
Initial commit of generating random data
@frewsxcv frewsxcv merged commit fc4aa04 into jazzband:master Aug 11, 2015
@frewsxcv frewsxcv mentioned this pull request Aug 11, 2015
@frewsxcv
Copy link
Collaborator

Just released 1.3.0 that includes this change

https://pypi.python.org/pypi/geojson/1.3.0

Thanks again!

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Aug 15, 2015
1.3.0 (2015-08-11)
------------------

- Add utility to generate geometries with random data

  - jazzband/geojson#60
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.

2 participants