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

Refactor EntitySet.find_path(...) #295

Merged
merged 10 commits into from Oct 29, 2018
Merged

Refactor EntitySet.find_path(...) #295

merged 10 commits into from Oct 29, 2018

Conversation

kmax12
Copy link
Contributor

@kmax12 kmax12 commented Oct 27, 2018

This PR simply rewrites the bread first search algorithm we use to find a path between two nodes.

Our existing approach defined an new class to do it, which resulted in bloated code.

The changes here remove that extra class and adds comments to create cleaner and more understandable code.

@kmax12 kmax12 changed the title Refactor find path Refactor EntitySet.find_path(...) Oct 27, 2018
@kmax12 kmax12 requested a review from rwedge October 29, 2018 16:35
@codecov-io
Copy link

codecov-io commented Oct 29, 2018

Codecov Report

Merging #295 into master will increase coverage by 0.02%.
The diff coverage is 97.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #295      +/-   ##
==========================================
+ Coverage   95.01%   95.04%   +0.02%     
==========================================
  Files          71       71              
  Lines        7627     7621       -6     
==========================================
- Hits         7247     7243       -4     
+ Misses        380      378       -2
Impacted Files Coverage Δ
...uretools/tests/entityset_tests/test_es_metadata.py 96.7% <100%> (+0.45%) ⬆️
featuretools/entityset/entityset.py 94.98% <96%> (+0.02%) ⬆️
featuretools/utils/gen_utils.py 88.37% <0%> (+2.32%) ⬆️

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 5a7ad62...23855c0. Read the comment docs.

Copy link
Collaborator

@rwedge rwedge left a comment

Choose a reason for hiding this comment

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

The docstring for this method still references BaseEntitySet, which should probably be changed. The description of the return values also refers to include_num_forward as include_forward_distance.

start_node = BFSNode(start_entity_id, None, None)
queue = [start_node]
nodes = {}
# Search for bath using BFS to get the shortest path.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Search for path

featuretools/entityset/entityset.py Show resolved Hide resolved
for r in next_relationships:
queue.append(current_path + [r])

visited.add(next_entity_id)

raise ValueError(("No path from {} to {}! Check that all entities "
.format(start_entity_id, goal_entity_id)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not tested according to codecov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

:func:`BaseEntitySet.find_forward_path`
:func:`BaseEntitySet.find_backward_path`
:func:`EntitySet.find_forward_path`
:func:`EntitySet.find_backward_path`
"""
if start_entity_id == goal_entity_id:
if include_num_forward:
return [], 0
else:
return []
Copy link
Collaborator

Choose a reason for hiding this comment

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

This else case not tested according to codecov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@rwedge
Copy link
Collaborator

rwedge commented Oct 29, 2018

Looks good

@rwedge rwedge merged commit 32cec5e into master Oct 29, 2018
@rwedge rwedge mentioned this pull request Oct 31, 2018
@kmax12 kmax12 deleted the refactor-find-path branch December 1, 2018 16:49
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

3 participants