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

feat(pipeline) : Ensure the zone_diffusion_codes for DROM/COM #288

Merged
merged 6 commits into from
Sep 19, 2024

Conversation

vperron
Copy link
Contributor

@vperron vperron commented Sep 11, 2024

There were cases where we did have the zone_diffusion_code set to "97" which can't be right, as it would mean that a given service coyuld be available across the oceans.

Let's fix it and set the correct, 3-digit department number.

This will also enable their search as we now (since the "new" communes) search for a match agains commune.departement, which can be 3 digits.

I would love to add a data test for this, but not sure how useful that is. So far, CHECK_SERVICE_ERRORS() is only semantic but does not validate for instance that the zone_diffusion_code is a correct department number if the zone_diffusion_type is "department", and so on.

If we wanted to validate this, what should be the correct way to proceed?

@@ -15,7 +15,7 @@ departements AS (
),

-- TODO: Refactoring needed to be able to do geocoding per source and then use the result in the mapping
services_with_zone_diffusion AS (
services_with_zone_diffusion_after_geocoding AS (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NDA : Je le nomme comme ça car dans ce bloc, on s'attache à appliquer les codes et noms des zones de diffusion SUITE au geocoding (on considère qu'à cette étape, les codes INSEE sont mieux résolus et par conséquent que les zones de diffusion seront plus précises en les appliquant à partir des codes INSEE post-geocoding)

@vperron
Copy link
Contributor Author

vperron commented Sep 12, 2024

Vu avec @vmttn :

  • ajouter un unit test pour vérifier la logique du code (en particulier passage par les différentes étapes de setting des ZD, etc)
  • ajouter un data test séparé qui vérifie (cf https://docs.getdbt.com/reference/resource-configs/where) que dans le cas de tel ou tel type zone de diffusion, on a bien des codes qui correspondent à une valeur correcte d'EPCI, commune, departement, region...

Copy link
Contributor

@vmttn vmttn left a comment

Choose a reason for hiding this comment

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

+1 pour les tests à venir :)

@vperron
Copy link
Contributor Author

vperron commented Sep 17, 2024

Etude d'impact statistique:

  • on passe de 138 lignes (action logement) avec un code de ZD à 97, à 0 lignes: elles ont maintenant un véritable code et nom de département.

  • on a un changement des choses en ce qui concerne les départements dans les DOM/TOM:

    • les noms des départements et communes sont canoniques et non plus exotiques
    • lorsque les services ont une géoloc mauvaise, leur ZD devient mauvaise
      ==> Cela ne concernant que l'ODSPEP, on décide de sortir ODSPEP de ce traitement.
  • Meme constat pour les ZD "vides" alors que elles ont un type de ZD "commune" ou "departement" : on ne "corrige" que pour une trentaine de lignes sur les ~12000 concernées par une ZD vide.
    ==> On ne peut pas "corriger" car les adresses en question n'ont en fait pas de code INSEE, même post-géocodage. Le vrai traitement est:

  • retenter le géocodage jusqu'a ce qu'un code INSEE se montre

  • en attendant potentiellement proposer de retirer les records qui ont un type de ZD mais pas de valeur associée

@vperron
Copy link
Contributor Author

vperron commented Sep 17, 2024

Vu avec @vmttn , on fait une exception pour ODSPEP et on fera un ticket supplémentaire plus tard pour rationaliser encore nos données d'adresse : mettre à 0 celles qui sont "mal" géocodées (en-dessous de 0.8)

@vperron vperron marked this pull request as draft September 18, 2024 13:28
@vperron vperron force-pushed the vperron/fix-departement-drom branch 2 times, most recently from a95c124 to 35e63b2 Compare September 18, 2024 14:34
@vperron vperron marked this pull request as ready for review September 19, 2024 09:31
@vmttn vmttn force-pushed the vperron/fix-departement-drom branch from 35e63b2 to 0710b2d Compare September 19, 2024 14:09
vperron and others added 3 commits September 19, 2024 16:37
There were cases where we did have the zone_diffusion_code set to "97"
which can't be right, as it would mean that a given service coyuld be
available across the oceans.

Let's fix it and set the correct, 3-digit department number.

This will also enable their search as we now (since the "new" communes)
search for a match agains commune.departement, which can be 3 digits.

There is also now a complete data validation that leaves the errors as a
specific table in the public_dbt_test__audit schema.
@vmttn vmttn force-pushed the vperron/fix-departement-drom branch from d561014 to e8ffb87 Compare September 19, 2024 14:37
@vmttn vmttn merged commit dd1d76c into main Sep 19, 2024
5 of 6 checks passed
@vmttn vmttn deleted the vperron/fix-departement-drom branch September 19, 2024 14:38
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.

3 participants