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

Fix map_geometries() loss of feature IDs #128

Merged
merged 3 commits into from
Jul 13, 2019
Merged

Conversation

bryik
Copy link
Contributor

@bryik bryik commented Jul 4, 2019

I noticed that a Feature passed to geojson.utils.map_tuples() loses it's ID. Here is a simple replication.

The cause seems to be that IDs are not included in the dict returned by geojson.utils.map_geometry().

This PR:

  • adds the ID to the dict aca22cb
  • adds a test to check ID is present after mapping f3e876a

@codecov
Copy link

codecov bot commented Jul 4, 2019

Codecov Report

Merging #128 into master will increase coverage by 1.78%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
+ Coverage   93.33%   95.11%   +1.78%     
==========================================
  Files          10       10              
  Lines         345      348       +3     
  Branches       69       70       +1     
==========================================
+ Hits          322      331       +9     
+ Misses         14        9       -5     
+ Partials        9        8       -1
Impacted Files Coverage Δ
geojson/utils.py 87.96% <75%> (+6.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e96c0e9...20880c0. Read the comment docs.

@bryik
Copy link
Contributor Author

bryik commented Jul 4, 2019

I don't know what codecov/patch is or why it is failing.

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

It's telling to improve the cov little bt more.

@rayrrr
Copy link
Member

rayrrr commented Jul 11, 2019

Hi @bryik, thank you for your contribution. However, I'm leaning against making this change, because I don't know that id should be returned by those functions.

If you read the docstrings (the main comments directly under each function declaration) in https://github.com/jazzband/python-geojson/blob/master/geojson/utils.py then you will see that the various utility functions in this repo are only intended to return partial data, not a full representation of the input object.

So, my understanding is that the utility functions are not intended to be a "put your object in one end and get it out the other transformed" functions, but "give me your object and I'll give you some transformed data you can add to it" functions.

Would using them this way meet your needs? In other words, change the second half of your example to:

geojson_feature = geojson.Feature(
    id=example["id"],
    geometry=geojson.Point(example["geometry"]["coordinates"]),
    properties=example["properties"])

# The ID is present.
print("id before map:", geojson_feature["id"])

# Map over the feature's tuples (without doing anything).
geojson_feature['mapped_geometry'] = geojson.utils.map_tuples(lambda t: t, geojson_feature)

# The ID and mapped data are now both available on the original object!
try:
    print("id:", geojson_feature["id"])
except KeyError:
    print("ID is missing!")

@bryik
Copy link
Contributor Author

bryik commented Jul 12, 2019

Hello @rayrrr. I'm not quite sure I understand your proposal.

geojson_feature['mapped_geometry'] = geojson.utils.map_tuples(lambda t: t, geojson_feature)

results in a geojson_feature that looks like:

{
  "geometry": {
    "coordinates": [-77.1291115237051, 38.7993076720178, 0],
    "type": "Point"
  },
  "id": "0",
  "mapped_geometry": {
    "geometry": {
      "coordinates": [-77.1291115237051, 38.7993076720178, 0],
      "type": "Point"
    },
    "properties": {
      "line": "blue",
      "marker-col": "#0000ff",
      "marker-sym": "rail-metro",
      "name": "Van Dorn Street"
    },
    "type": "Feature"
  },
  "properties": {
    "line": "blue",
    "marker-col": "#0000ff",
    "marker-sym": "rail-metro",
    "name": "Van Dorn Street"
  },
  "type": "Feature"
}

See this repl.

Are you saying that this behaviour is intended?

@bryik
Copy link
Contributor Author

bryik commented Jul 12, 2019

Looking at the docs again, I see map_tuple() says it "maps a function over all coordinates and returns a geometry of the same type" which implies Features should not be passed to it or if they are, that users should be prepared to receive only a geometry back.

However, the docstring of map_tuples() says it takes "A geometry or feature to extract the coordinates from" and Features have an explicit case in the code that does not raise an exception. Also when given a Feature, map_tuples() returns a Feature and not a geometry.

@rayrrr
Copy link
Member

rayrrr commented Jul 13, 2019

@bryik your additional notes helpful, and you are correct. Merging your PR in its current form. I was basing my earlier comments off of misleading docstrings, sorry. The docs/docstrings for utils.py functions need improvement...we have a related open issue: #110. While I would like these changes to be a bit more idiomatic, I will handle that in a quick follow-up PR and will give you a heads up on that. Thanks again.

@rayrrr rayrrr merged commit 813117e into jazzband:master Jul 13, 2019
@bryik
Copy link
Contributor Author

bryik commented Jul 13, 2019

@rayrrr Happy to help :-)

Thanks for the review and for maintaining this project!

@rayrrr
Copy link
Member

rayrrr commented Jul 13, 2019

@bryik #130 is the follow-up PR, have a look.

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.

3 participants