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

Adds the ability to preserve the feature IDs and properties when creating zonal stats without geojson #152

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

doclements
Copy link

No description provided.

Repository owner deleted a comment from coveralls Aug 20, 2017
Repository owner deleted a comment from coveralls Aug 20, 2017
Repository owner deleted a comment from coveralls Aug 20, 2017
Copy link
Owner

@perrygeo perrygeo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Some changes suggested below.

feature_stats['properties'] = feat['properties']
if preserve_ids:
if 'id' in feat['properties']:
feature_stats['id'] = feat['properties']['id']
Copy link
Owner

@perrygeo perrygeo Aug 20, 2017

Choose a reason for hiding this comment

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

Three suggestions for this section:

  1. We can simplify the branching logic here
if preserve_properties and 'properties' in feat:
    ...
  1. If the id is already part of properties, the preserve_ids implementation is superfluous. The true id, according to the GeoJSON spec, is at the top level of the feature.
if preserve_ids and 'id' in feat:
    feature_stats['id'] = feat['id']
  1. Because this code deals with massaging the output data, not directly with zonal stats itself, I'd like to move it to the bottom of the function. This should share code with the application of geojson_out (geojson_out is basically equivalent to preserve_properties + preserve_ids + preserve_geometry)

Copy link
Owner

Choose a reason for hiding this comment

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

Also, we'll need to add the new arguments to the function docstring.

Copy link
Author

Choose a reason for hiding this comment

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

github decided not to give any notifications for this so i've only just noticed the comments. I'll look into making the changes as discussed.

thanks

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

Successfully merging this pull request may close these issues.

4 participants