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

Rjf/expose graph ops #65

Merged
merged 9 commits into from
Dec 8, 2023
Merged

Rjf/expose graph ops #65

merged 9 commits into from
Dec 8, 2023

Conversation

robfitzgerald
Copy link
Collaborator

@robfitzgerald robfitzgerald commented Dec 7, 2023

this PR adds a few simple commands to inspect graph attributes from CompassApp and CompassAppWrapper / python CompassApp. that includes:

    def graph_edge_origin(self, edge_id: int) -> int:
    def graph_edge_destination(self, edge_id: int) -> int:
    def graph_edge_distance(self, edge_id: int, distance_unit: Optional[str]) -> float:
    def graph_get_out_edge_ids(self, vertex_id: int) -> List[int]:
    def graph_get_in_edge_ids(self, vertex_id: int) -> List[int]:

each time one of these methods is called, we clone the Arc<DriverReadOnlyLock<Graph>>, which is a small performance hit which may be acceptable for some one-off operations but may slow things down for large batch queries. if that becomes a bottleneck we can add some batch operations that follow the pattern of these methods to amortize the clone runtime hit.

example

using the golden, co test case in the notebook example, i have this for my edges-compass.csv.gz file:

      edge_id  src_vertex_id  dst_vertex_id  distance
0           0              0            429   474.918
1           1              2              3    71.722
2           2              2            173    33.589
3           3              2            178   111.341
4           4              3              2    71.722
...       ...            ...            ...       ...
1755     1755            743            474    59.098
1756     1756            743            744    25.754     <---- this row in example
1757     1757            744            732    31.905
1758     1758            745            574    21.303
1759     1759            745            663   217.939

i run these queries and get this result:

(
    app.graph_edge_origin(1756),
    app.graph_edge_destination(1756),
    app.graph_edge_distance(1756, None),          # None is also the default argument
    app.graph_edge_distance(1756, 'meters'),       
    app.graph_edge_distance(1756, 'kilometers'),
    app.graph_edge_distance(1756, 'miles'),
    app.graph_get_out_edge_ids(743),
    app.graph_get_in_edge_ids(744)
)
(743,
 744,
 25.754,
 25.754,                  # meters is default
 0.025754000000000003,    # 25.75 / 1000
 0.0160062150410092,      # 25.75 / 1609
 [1755, 1756],            # includes test row id
 [1733, 1756])            # includes test row id

Closes #15.

@robfitzgerald
Copy link
Collaborator Author

i'll add an example of error handling for distance units here:

app.graph_edge_distance(1756, 'boog')
---------------------------------------------------------------------------
Exception                                 Traceback (most recent call last)
[/Users/rfitzger/dev/nrel/routee/routee-compass/docs/notebooks/open_street_maps_example.ipynb](https://file+.vscode-resource.vscode-cdn.net/Users/rfitzger/dev/nrel/routee/routee-compass/docs/notebooks/open_street_maps_example.ipynb) Cell 10 line 1
----> [1](vscode-notebook-cell:/Users/rfitzger/dev/nrel/routee/routee-compass/docs/notebooks/open_street_maps_example.ipynb#X40sZmlsZQ%3D%3D?line=0) app.graph_edge_distance(1756, 'boog')

File [~/dev/nrel/routee/routee-compass/python/nrel/routee/compass/compass_app.py:120](https://file+.vscode-resource.vscode-cdn.net/Users/rfitzger/dev/nrel/routee/routee-compass/docs/notebooks/~/dev/nrel/routee/routee-compass/python/nrel/routee/compass/compass_app.py:120), in CompassApp.graph_edge_distance(self, edge_id, distance_unit)
    [119](https://file+.vscode-resource.vscode-cdn.net/Users/rfitzger/dev/nrel/routee/routee-compass/docs/notebooks/~/dev/nrel/routee/routee-compass/python/nrel/routee/compass/compass_app.py:119) def graph_edge_distance(self, edge_id: int, distance_unit: Optional[str]) -> float:
--> [120](https://file+.vscode-resource.vscode-cdn.net/Users/rfitzger/dev/nrel/routee/routee-compass/docs/notebooks/~/dev/nrel/routee/routee-compass/python/nrel/routee/compass/compass_app.py:120)     return self._app.graph_edge_distance(edge_id, distance_unit)

Exception: could not deserialize distance unit 'boog'

Copy link
Collaborator

@nreinicke nreinicke left a comment

Choose a reason for hiding this comment

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

Cool, this makes sense to me. I left a small request for a comment.

) -> PyResult<f64> {
let du_internal_result: PyResult<Option<DistanceUnit>> = match distance_unit {
Some(du_str) => {
let mut enquoted = du_str.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wonder if we could add some comments here to explain why this has to be done or maybe create a simple ops function that does this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for real. i was fishing around for this trick and have run into this previously as well. i'll add a comment, as i'm not sure it's the best way to leverage serde to decode non-parameterized enums from a string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eh, @nreinicke, i changed my mind; i'll make a generic utility for it, and then leverage that in FromStr implementations for the Unit types. coming right up.

@robfitzgerald
Copy link
Collaborator Author

@nreinicke addressed your comment by making that a generic util and then using it in FromStr implementations for the Unit types.

Copy link
Collaborator

@nreinicke nreinicke left a comment

Choose a reason for hiding this comment

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

Sweet, looks great! Thanks for adding these in!

@robfitzgerald robfitzgerald merged commit 2c233da into main Dec 8, 2023
5 checks passed
@robfitzgerald robfitzgerald deleted the rjf/expose-graph-ops branch December 8, 2023 17:44
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.

expose graph operations to python
2 participants