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

Add support for automatic dict and set attributes in module io #1075

Merged
merged 13 commits into from
Oct 24, 2023

Conversation

ncotie
Copy link
Contributor

@ncotie ncotie commented Oct 18, 2023

Proposing some extra code support in module io for decoding stringified lists, dicts and sets automatically when running load_graphml(), allowing users to avoid having to specify ast.literal_eval as dtype for customized (non-OSM) node and/or edge attributes.

The proposed code expands existing code from _convert_edge_attr_types() that handles stringified lists as edge attributes.

This is linked to Issue #1067.

Changes are made in _convert_node_attr_types() and _convert_edge_attr_types(), adding the following to both:

    # check for stringified dicts or sets: both can be converted
    # to python objects using ast.literal_eval()
    if value.startswith("{") and value.endswith("}"):
        with contextlib.suppress(SyntaxError, ValueError):
            data[attr] = ast.literal_eval(value)

Tested with toy networks as in the issue description, for list, set and dict node and edge attributes.

osmnx/io.py Outdated Show resolved Hide resolved
osmnx/io.py Outdated Show resolved Hide resolved
osmnx/io.py Outdated
Comment on lines 428 to 440
# first, eval stringified lists, dicts or sets to convert them to objects
# edge attributes might have a single value, or a list if simplified
for attr, value in data.items():
# check for stringified lists
if value.startswith("[") and value.endswith("]"):
with contextlib.suppress(SyntaxError, ValueError):
data[attr] = ast.literal_eval(value)
# check for stringified dicts or sets: both can be converted
# to python objects using ast.literal_eval()
if value.startswith("{") and value.endswith("}"):
with contextlib.suppress(SyntaxError, ValueError):
data[attr] = ast.literal_eval(value)

Copy link
Owner

Choose a reason for hiding this comment

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

We previously only ran literal_eval on edge attributes. Is there a use case for node attributes? OSMnx does not produce any node attributes of type list, dict, or set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can describe my usage, if it makes sense to you. It's clearly all custom attributes, as I mentioned in the issue description, so not necessarily OSMnx scope.

I'm doing 15-minute city metrics, based on OSMnx with a lot of my own code on top. I fetch a walking network via OSMnx, then integrate amenity POI nodes into it, adding linking edges with lengths. The network nodes, apart from the amenities, are considered to be "residential locations", without getting into filtering details. Processing then obtains, among other things, shortest path distances, per residential node, to each amenity node. These are stored per node into a dict of dicts attribute keyed by amenity category and amenity node.
Accessible amenities are counted per residential node and edge, also, which allows for producing isochrone buffered geometries, for example.

It's true that I could have managed these outside the graph object using dicts, but I had wanted to make the work graph-oriented, and I believe it simplifies things, avoiding the need to keep separate objects consistent when doing things like truncating, etc, etc.

osmnx/io.py Outdated Show resolved Hide resolved
@gboeing
Copy link
Owner

gboeing commented Oct 19, 2023

CI tests are failing. You can check the failure here and correct it manually, or see the tests documentation.

@gboeing
Copy link
Owner

gboeing commented Oct 19, 2023

Tests are failing. Please lint and test your code locally before pushing. See the tests folder.

@ncotie
Copy link
Contributor Author

ncotie commented Oct 19, 2023

Geoff, apologies but the push earlier today surprised me, I didn't know that when I pushed to my own fork that it would update here immediately... Impressive integration in git...
As noted in the last merge, I created a local env from /tests/env-ci.yml, and looked to see how to test it. Called format.sh and lint-test.sh, which made two formatting changes automatically, but I saw that by coincidence, and I can't find any other results from the local test, so I can't tell if it passes properly now or not.

osmnx/io.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.83%. Comparing base (dfa7226) to head (3ff4730).

Additional details and impacted files
@@           Coverage Diff           @@
##               io    #1075   +/-   ##
=======================================
  Coverage   97.83%   97.83%           
=======================================
  Files          27       27           
  Lines        2493     2497    +4     
=======================================
+ Hits         2439     2443    +4     
  Misses         54       54           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gboeing gboeing marked this pull request as ready for review October 20, 2023 21:08
@ncotie
Copy link
Contributor Author

ncotie commented Oct 21, 2023

Geoff, tell me please if I should be doing anything specific with the PR now. I'm assuming the "review" is for you to do and that there's nothing needed from me for now. The "1 change requested" tagged just above is I think already commented in the discussion earlier, and I don't believe I need to take any action on it. If that's not correct, please let me know.

@gboeing gboeing self-requested a review October 22, 2023 20:03
@gboeing gboeing changed the base branch from main to io October 24, 2023 06:07
@gboeing gboeing merged commit 4d00af2 into gboeing:io Oct 24, 2023
7 checks passed
@ncotie
Copy link
Contributor Author

ncotie commented Oct 25, 2023

Thanks Geoff!

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