-
Notifications
You must be signed in to change notification settings - Fork 101
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
feat: implemented transfer distance validator #1958
base: master
Are you sure you want to change the base?
Conversation
@cka-y I know this isn't code review ready yet but I got excited...
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 0ad6a39 📊 Notices ComparisonNew Errors (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (4 out of 1801 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
Dropped Warnings (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1801 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the validator table and the test code. The output looks correct.
@cka-y - Looks great.
|
|
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit f1ba463 📊 Notices ComparisonNew Errors (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (4 out of 1801 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
Dropped Warnings (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1801 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 757f5c2 📊 Notices ComparisonNew Errors (0 out of 1809 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1809 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (4 out of 1809 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
Dropped Warnings (0 out of 1809 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1809 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cka-y just one question before approving, what happened to mdb-784? It doesn't exist in the +4 new warnings despite the transfer distance being 5463 km
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cka-y thanks for clarifying. Everything looks fine on the spec side.
@emmambd If we change the And, to go the other way, is it advisable to have the distance set in the notice name? Are we sure we will never decide that, for example, 1 km is enough of a distance to warrant an info instead of 2km? |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 99fe817 📊 Notices ComparisonNew Errors (0 out of 1811 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1811 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (3 out of 1811 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
Dropped Warnings (0 out of 1811 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1811 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
Hey @jcpitre! Sorry, for the wait, I asked @skalexch to take a look at how this worked in the old deprecated validator first. In the old one, they only have 1 notice and the severity varies (as well as the message which dynamically displays the invalid transfer distance) depending on the result. I still think we should keep the behavior as is: |
Summary
This PR introduces an info notice when the transfer distance exceeds 2 km and a warning when it exceeds 10 km.
Expected Behavior
Using mdb-784, we observe the following results:


Distribution Analysis
The graph below shows the distribution of the max transfer distance across feeds in the Mobility Database.
Most transfer distances fall below the 2 km info threshold, with only a few exceeding the error threshold of 10 km.
Top 3 Largest Distances
While mdb-784 contains an extreme outlier, most feeds remain within the defined thresholds (2 km for warnings, 10 km for errors).
gradle test
to make sure you didn't break anything