Skip to content

Fix time calculations in insert_one_edge#278

Merged
mpadge merged 8 commits intoUrbanAnalyst:mainfrom
leoniedu:fix_time_insert_one_edge
Mar 10, 2025
Merged

Fix time calculations in insert_one_edge#278
mpadge merged 8 commits intoUrbanAnalyst:mainfrom
leoniedu:fix_time_insert_one_edge

Conversation

@leoniedu
Copy link
Copy Markdown
Contributor

@leoniedu leoniedu commented Mar 7, 2025

Propose fix to #277

Basically the code was not updating the time values for index + 1

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.07%. Comparing base (4bffc7f) to head (b347867).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
- Coverage   94.12%   94.07%   -0.05%     
==========================================
  Files          51       51              
  Lines        6821     6823       +2     
==========================================
- Hits         6420     6419       -1     
- Misses        401      404       +3     

☔ 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.

Comment thread R/graph-functions.R Outdated
Comment thread R/graph-functions.R Outdated
Comment thread R/graph-functions.R
Comment thread R/graph-functions.R Outdated
Comment thread R/graph-functions.R Outdated
@mpadge
Copy link
Copy Markdown
Member

mpadge commented Mar 7, 2025

That's great @leoniedu, thank you very much for finding that bug. I've modified your code a bit, only to try to make it a bit clearer what is actually happening. Under each of my comments, you should see a button to "Commit Suggestion". If you click each of those, and maybe reformat a little, you should see something like this:

if (!is.na (gr_cols$time)) {
    graph [index, gr_cols$time] <- d1 * time_scale
    graph [index + 1, gr_cols$time] <- d2 * time_scale
    graph [index, gr_cols$time_weighted] <- d1 * time_scale * time_wt
    graph [index + 1, gr_cols$time_weighted] <- d2 * time_scale * time_wt
}

I think having the explicit d1 and d2 values repeated there makes the relationship to distances more clear. Let me know when that's done, and should be good to merge.

Great that you included tests too! If i had have had that in there, this would never have happened. Nice!

leoniedu and others added 5 commits March 7, 2025 12:18
Co-authored-by: mark padgham <mark.padgham@email.com>
Co-authored-by: mark padgham <mark.padgham@email.com>
Co-authored-by: mark padgham <mark.padgham@email.com>
Co-authored-by: mark padgham <mark.padgham@email.com>
Co-authored-by: mark padgham <mark.padgham@email.com>
Comment thread R/graph-functions.R Outdated
Co-authored-by: mark padgham <mark.padgham@email.com>
@mpadge mpadge merged commit 3566ff9 into UrbanAnalyst:main Mar 10, 2025
mpadge added a commit that referenced this pull request Mar 10, 2025
mpadge added a commit that referenced this pull request Mar 10, 2025
Plus one styler fix
mpadge added a commit that referenced this pull request Mar 10, 2025
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