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

Geometry sequence as features #14

Merged
merged 7 commits into from Nov 7, 2016
Merged

Geometry sequence as features #14

merged 7 commits into from Nov 7, 2016

Conversation

perrygeo
Copy link
Contributor

This PR allows @features_in_arg to take a sequence of geometry objects on stdin and casts them to features with empty properties.

Resolves #13

if 'coordinates' in obj.keys():
return True
else:
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

return 'coordinates' in obj is the way to go here, yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, return <boolean expression> is definitely the way to go.

In terms of 'coordinates' in obj vs coordinates in obj.keys(): I prefer the explicit call to the keys method because it enforces that the object is a mapping and won't match this is a geometry just because it has coordinates in the string.

@sgillies sgillies added this to the 0.5 milestone Jan 4, 2016
@sgillies
Copy link
Contributor

sgillies commented Jan 4, 2016

Yes, but check out the performance hit from calling keys():

>>> timeit.timeit(stmt="'coordinates' in obj", setup="obj={'coordinates': [0, 0]}")
0.04499248508363962
>>> timeit.timeit(stmt="'coordinates' in obj.keys()", setup="obj={'coordinates': [0, 0]}")
0.12553653097711504

@perrygeo
Copy link
Contributor Author

perrygeo commented Jan 4, 2016

So if is_geometry was intended to be a reusable public function, I'd want to assert that it was a mapping with a coordinate key, not a string containing "coordinate". However, this is more of a private utility function to make the code read better. Should we just inline 'coordinates' in obj and forget the function?

@sgillies
Copy link
Contributor

sgillies commented Jan 5, 2016

@perrygeo yes to that.

@perrygeo
Copy link
Contributor Author

I had to resolve some merge conflicts and take a slightly different approach.

@sgillies 👀 ?

yield newfeat


def to_feature(obj):
Copy link
Contributor

Choose a reason for hiding this comment

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

@perrygeo I've got no immediate use for what I'm about to suggest: but what about adding a properties keyword argument to this function, the value of which would be copied to the result's properties member?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we punt on that and cover that use case with a more general tool like the proposed fio calc?

@perrygeo
Copy link
Contributor Author

perrygeo commented Nov 7, 2016

@sgillies anything need to move on this before merging?

@sgillies sgillies merged commit 892bd5b into master Nov 7, 2016
@sgillies sgillies deleted the geoms-as-features branch November 7, 2016 14:50
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