Skip to content

Conversation

@bz247
Copy link
Contributor

@bz247 bz247 commented Aug 6, 2020

Road networks are usually stored as dataframes. Therefore, it is easier to create a graph directly from the dataframes.

Currently, the graph needs to be created in the following sequence: dataframe --> matrix --> save .mtx --> read .mtx into graph.

The proposed changes will create graph using: dataframe --> graph.

@bz247
Copy link
Contributor Author

bz247 commented Aug 10, 2020

When the start/end node ids in the edges dataframe are not named sequentially, e.g., given the edges dataframe below

Start End Weight
1 5 100
5 10 200

The number of edges is 2 without problem. The number of vertices, should it be 10 or 3? It needs to be 10 to use the same node id as in the dataframe. However, there are only actually 3 nodes.

@bz247 bz247 marked this pull request as ready for review August 10, 2020 05:58
@kks32 kks32 self-assigned this Aug 10, 2020
Copy link
Contributor

@kks32 kks32 left a comment

Choose a reason for hiding this comment

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

Looks good. You may need to run through clang-format to format the c++ code.

@bz247
Copy link
Contributor Author

bz247 commented Aug 11, 2020

I have run the src and include folders through clang-format.

bz247 and others added 2 commits August 12, 2020 15:25
Co-authored-by: Krishna Kumar <3963513+kks32@users.noreply.github.com>
@kks32
Copy link
Contributor

kks32 commented Aug 13, 2020

Could you please check clang-format output?

@bz247
Copy link
Contributor Author

bz247 commented Aug 15, 2020

clang-format splits long function annotation \param[in] to new lines at wrong places. E.g.,
+//! \param[in] edge_starts name of the dataframe column containing edge start
+//! nodes \param[in] edge_ends name of the dataframe column containing edge end

  • I think it is desirable not to have a new \param[in] that directly follows the previous one on the same line
  • I couldn't stop the splitting after adding variations of CommentPragmas: '\param[in]' in include/.clang-format.
  • Shall I use // clang-format on/off?

@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2020

Codecov Report

Merging #5 (6cda7e7) into develop (38cb04c) will decrease coverage by 1.70%.
The diff coverage is 0.00%.

❗ Current head 6cda7e7 differs from pull request most recent head 2a984a1. Consider uploading reports for the commit 2a984a1 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop       #5      +/-   ##
===========================================
- Coverage    83.94%   82.23%   -1.70%     
===========================================
  Files            6        6              
  Lines          386      394       +8     
===========================================
  Hits           324      324              
- Misses          62       70       +8     
Impacted Files Coverage Δ
include/graph.h 100.00% <ø> (ø)
src/graph.cc 89.08% <0.00%> (-6.42%) ⬇️

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 38cb04c...2a984a1. Read the comment docs.

Copy link
Contributor

@kks32 kks32 left a comment

Choose a reason for hiding this comment

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

why are we removing this file?

@kks32 kks32 self-requested a review August 20, 2020 15:13
Copy link
Contributor

@kks32 kks32 left a comment

Choose a reason for hiding this comment

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

Could you please add .clang-format back in?

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.

5 participants