Skip to content

Add nodes by edge: attempt at #294 - #296

Open
leoniedu wants to merge 4 commits into
UrbanAnalyst:mainfrom
leoniedu:add-nodes-by-edge
Open

Add nodes by edge: attempt at #294#296
leoniedu wants to merge 4 commits into
UrbanAnalyst:mainfrom
leoniedu:add-nodes-by-edge

Conversation

@leoniedu

Copy link
Copy Markdown
Contributor

Implement #294

@codecov

codecovBot commented Apr 4, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 95.73171% with 7 lines in your changes missing coverage. Please review.

Project coverage is 94.28%. Comparing base (0980407) to head (d19ea03).

Files with missing linesPatch %Lines
R/match-points-by-edge.R95.73%7 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #296 +/- ##
==========================================
+ Coverage 94.13% 94.28% +0.15% 
==========================================
Files 52 53 +1 Lines 6893 7057 +164 ==========================================
+ Hits 6489 6654 +165 + Misses 404 403 -1 

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mpadge

Copy link
Copy Markdown
Member

Thanks @leoniedu, but your commits have entirely reformatted the code, so I can't see what you've done. Can you revert the code back to what it was, and then ensure you implement your changes only. It would also still be good to have some example code to work with, so if you can, could you please paste that in the issue #294. Thank you!

@leoniedu

Copy link
Copy Markdown
ContributorAuthor

Yes, it's more of a proof of concept, since I was having a hard time figuring out add_nodes_to_graph and how to adapt it for the by edge logic.

I have a couple of questions. Could you answer them, to help me get to the correct path? If this is not the appropriate channel to do so, let me know.

  1. Why do we create edges from point to edge ends when (any (dmat [upper.tri (dmat)] < dist_tol)), even when intersections_only = TRUE?

1.1) Related, if the point is too close (i.e. < dist_tol) to any of the edge vertices, we shouldn't be splitting or creating vertices, right?

  1. Is the order of the rows in the graph important? How?

  2. Should we perhaps start by generalizing dodgr_insert_vertex and/or insert_one_edge so that it handles multiple x/y coordinates for the splits ? This could make the code more modular.

Sign up for freeto 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

@leoniedu@mpadge