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

deleted service roads #161

Merged
merged 3 commits into from
Aug 18, 2024
Merged

deleted service roads #161

merged 3 commits into from
Aug 18, 2024

Conversation

Kryndlea
Copy link
Contributor

No description provided.

@Kryndlea Kryndlea requested a review from jGaboardi August 15, 2024 13:40
@Kryndlea
Copy link
Contributor Author

Waiting for Wuhan data to be uploaded so it can be corrected as well..

@jGaboardi
Copy link
Collaborator

@Kryndlea Great work!

How deep of a review would you like here? [1] Shall I actually inspect, or [2] (for sanity sake) can we all assume all is well?

(please be [2] ... 😂 )

cc @martinfleis @anastassiavybornova

If the [2] scenario, I will update the tests accordingly to get them green and we will be good to merge!

🥳

xref:

@martinfleis
Copy link
Contributor

Could you just skim it?

@jGaboardi
Copy link
Collaborator

Could you just skim it?

You mean skip the review or add skips to the failing tests (for now)?

@martinfleis
Copy link
Contributor

I meant, do [1] please but just very very quickly without looking into details

@jGaboardi
Copy link
Collaborator

Ah! LOL. I read "skim" as "skip".

I will see about getting to this today.

@jGaboardi
Copy link
Collaborator

  • @Kryndlea All seems well will the data itself. 🥳
  • @martinfleis Shall I update the failing tests? I assume the answer is "yes", but want to triple confirm.

@martinfleis
Copy link
Contributor

Yes. 3x.

@jGaboardi
Copy link
Collaborator

As a quadruple check with @Kryndlea & @dancejod. These are the current counts of road features in the Original vs. Manual roads:

================================
* Aleppo *
--------------
Records
  Orig: 78,908
  Manu: 38,772

================================
* Auckland *
--------------
Records
  Orig: 60,364
  Manu: 8,640

================================
* Bucaramanga *
--------------
Records
  Orig: 79,317
  Manu: 14,170

================================
* Douala *
--------------
Records
  Orig: 84,819
  Manu: 29,252

================================
* Liège *
--------------
Records
  Orig: 79,907
  Manu: 13,508

================================
* Salt Lake City *
--------------
Records
  Orig: 50,917
  Manu: 11,032

Some of these counts are significantly lower than initially manually simplified data that included service roads, but I was a touch surprised at how actually much. Taking SLC for example, previously we had 43,955 and now 11,032. The difference seems to be more pronounced in the global south, so this might jive exactly with original attribute data quality from OSM.

FAILED core/tests/test_utils.py::test_read_manual[Aleppo-41481] - assert 38772 == 41481
FAILED core/tests/test_utils.py::test_read_manual[Auckland-35650] - assert 8640 == 35650
FAILED core/tests/test_utils.py::test_read_manual[Bucaramanga-20347] - assert 14170 == 20347
FAILED core/tests/test_utils.py::test_read_manual[Douala-30647] - assert 29252 == 30647
FAILED core/tests/test_utils.py::test_read_manual[Li\xe8ge-25133] - assert 13508 == 25133
FAILED core/tests/test_utils.py::test_read_manual[Salt Lake City-43955] - assert 11032 == 43955

I think all is good here, but wanted to get this note here for posterity before I update the testing values.

cc @martinfleis @anastassiavybornova

@jGaboardi
Copy link
Collaborator

  • Updating testing values & imaging gets CI green again (still need to XFAIL several until the parenx data gets worked out), but I'd like more eyes on this before merging considering my comment above.
  • Added a checker notebook in for comparing original vs. manual plotting for quick local examination

@jGaboardi
Copy link
Collaborator

I'll merge later today unless I hear any objections. Probably not sooner than 2-3 hours from now.

@anastassiavybornova
Copy link
Collaborator

i was going to do a check from my side tomorrow but also fine with it being merged, trust you

@jGaboardi
Copy link
Collaborator

i was going to do a check from my side tomorrow but also fine with it being merged, trust you

Oh! I don't think we are in a rush -- and better to have another check, even if superficially. I will wait for your approval.

@martinfleis
Copy link
Contributor

I think that the large difference in counts is primarily due to removal of nodes of degree 2 as part of the manual process.

@anastassiavybornova
Copy link
Collaborator

Wuhan will come later right, if I understood correctly? I checked all other data sets, looks good to me!

@martinfleis
Copy link
Contributor

Wuhan will come later right, if I understood correctly?

Yes, as soon as @dancejod will emerge from vacation.

@martinfleis martinfleis merged commit 19d3e95 into main Aug 18, 2024
7 checks passed
@martinfleis martinfleis deleted the corrected_files branch August 18, 2024 12:26
@jGaboardi jGaboardi mentioned this pull request Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants