Skip to content

Ajout nouveaux facteurs et MAJ données anciens#179

Merged
ludovicdmt merged 26 commits intodevfrom
data-new-factors
Apr 8, 2025
Merged

Ajout nouveaux facteurs et MAJ données anciens#179
ludovicdmt merged 26 commits intodevfrom
data-new-factors

Conversation

@ludovicdmt
Copy link
Member

@ludovicdmt ludovicdmt commented Apr 1, 2025

  • Ajout des facteurs place PMR + station auto-partage + ponts comme non-plantable
  • MAJ des données pour les facteurs : slt, réseau fibre métropole
  • Simplification de geom pour les LCZ
  • Simplifcation génération MVT

Pour la fibre fusionne l'ancien et les nouveaux fichiers qui se complètent.

Dans le fichier transmis par la voirie, il manque un champ dans les données SLT. En attente de leur réponse.

@ludovicdmt ludovicdmt linked an issue Apr 1, 2025 that may be closed by this pull request
2 tasks
@ludovicdmt ludovicdmt changed the title AJout nouveaux facteurs et MAJ données anciens Ajout nouveaux facteurs et MAJ données anciens Apr 2, 2025
@ludovicdmt
Copy link
Member Author

ludovicdmt commented Apr 3, 2025

image

Avec les nouvelles valeurs de threshold et les MVT générées en DB

Comment on lines 13 to 21
const score = computed(() => {
if (props.index < -3) return 0
if (props.index < -2) return 2
if (props.index < -0.5) return 4
if (props.index < 0.75) return 6
if (props.index < 1.5) return 8
return 10
})

Copy link
Member Author

Choose a reason for hiding this comment

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

J'ai gardé ici un score entre 0 et 10 au clic par pédagogie (thresholds les mêmes que ceux de couleur)

C'est sans doute à raffiner avec des tests utilisateurs pour le nombre de classe / découpage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Non, tu ne devrais pas définir deux fois la même chose dans le front & le back, si besoin de cette échelle, tu peux ajouter une requête api pour l'obtenir ? O

Une solution serait que ce que tu stockes en base soit réellement ces valeurs là dans normalized indice

Comme les score sont associés à des labels, peut-être peut-on simplement ne pas afficher le score mais seulemnet le label ?

Copy link
Member Author

@ludovicdmt ludovicdmt Apr 4, 2025

Choose a reason for hiding this comment

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

Pour l'instant favorable à afficher le score car c'est ce qu'a défini Geoffrey dans ses maquettes mais on peut en discuter avec lui.

J'ai bougé le calcul du normalized indice pour que ce soit directement la valeur seuillée vu qu'on ne sert nul part de la valeur brute, ce qui résous le doublon.

Comment on lines +23 to +28
if (score.value === 0) return "Plantation impossible"
if (score.value === 2) return "Plantation très contrainte"
if (score.value === 4) return "Plantation contrainte"
if (score.value === 6) return "Plantation neutre"
if (score.value === 8) return "Plantation favorisée"
return "Plantation très favorisée"
Copy link
Member Author

Choose a reason for hiding this comment

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

Ces nommages proviennent du calque V1 et avaient été choisis avec des acteurs de terrain

Copy link
Contributor

Choose a reason for hiding this comment

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

D'une manière générale, s'il y a besoin de commenter une PR avec des commentaires, c'est que ces derniers auraient mérité leur place dans le code.

Dans le code, grâce à Quentin notamment, on aurait utilisé un enum ici.

Copy link
Contributor

@Marc-AntoineA Marc-AntoineA left a comment

Choose a reason for hiding this comment

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

Remarques générales :

  • sur la partie données, toujours une frustration de ma part de cette absence de tests mais ok. Mettre à jour la doc ?
  • sur le refacto, quand tu déplaces puis modifie des fonctions, tu peux le faire en deux fois avec un nom de commit ultra explicite. Comme ça, en ciblant les bons commits, je peux voir quelles modifs ont été faites ;
  • Je pense tu peux déjà ajouter une entrée dans le changelog avec des explications et capture d’écrans zoomées (avant/après) en prenant en compte tes nouvelles sources de données.

Ça fait plaiz en tout cas de voir ces améliorations !!

None

Reference:
https://makina-corpus.com/django/generer-des-tuiles-vectorielles-sur-mesure-avec-django
Copy link
Contributor

Choose a reason for hiding this comment

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

J'ai l'impression que tu as grandement simplifié le code, sans ajouter de nouvelle dépendance.

Est-ce que tu as aussi constaté eu des gains de temps de calcul ?
(c'est cool niveau comm de dire qu'on a eu des améliorations de perfs !)

Copy link
Member Author

Choose a reason for hiding this comment

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

Non pas de gain, ni de différence. Juste maintenant c'est mapbox_vector_tile qui fait les opérations au lieu que ce soit nous à la main.

{
"name": "SLT",
"file": "sltmateriel.geojson",
"file": "voirie.gpkg",
Copy link
Contributor

Choose a reason for hiding this comment

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

Est-ce le bon nom de fichier ? Dans ce cas, pourquoi avoir également ajouté (en commentaire) un autre fichier voirie.cpkg?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oui c'est le bon nom. gpkg c'est un fichier qui contient plusieurs layers (quand geojson) n'en a qu'une.
Valérian des SIG à la voirie m'a fait un export en gpkg de toutes les données les concernant, quand la dernière fois ils avaient fait des exports séparés en geojson.
Il a juste oublié un champ indispensable pour les données SLT donc je garde l'ancien fichier en attendant qu'il me refasse l'export et que je puisse switcher sur la version commentée (j'ai pas envie de ré-écrire la même chose dans 1 semaine donc je stocke là temporairement en commentaire).

"factors": ["Rsx aériens ERDF"],
"output_type": "LINESTRING",
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Fichier data.md à mettre à jour non ?

"name": "Réseaux aériens Enedis",
"file": "rsx_aerien_enedis.geojson",
"actions": [{"buffer_size": 1, "union": True}],
"actions": [{"buffer_size": 1, "union": False}],
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi ce passage de True à False. Est-ce qu'il y avait un bug avant ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Non avant j'explosais les géométries en suivant une grille régulière pour simplifier les traitements suivants en me permettant de travailler avec juste des bouts de géom.
Je me suis rendu compte qu'en gardant juste l'explosion inhérente aux données (donc pas d'union), ça va tout aussi vite pour les traitements suivants mais ça simplifie l'import des données.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pour du tel refacto, proposition de créer un dossier utils avec différents fichiers plutôt que des fichiers préfixés par utils.

},
)

def test_transform_to_tile_relative(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Supprimé car plus la méthode associée c'est ça ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oui les transformations correspondantes sont maintenant directement traitée par mapbox_vector_tile donc on se contente de tester le résultat final qui est les tuiles MVT et plus les étapes intermédiaires.

Copy link
Contributor

Choose a reason for hiding this comment

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

On pourait s'attendre à ce que le nombre de tests augmente et ne diminue pas :) Du coup, est-ce que tu aurais 2 tests unitaires pertinents à ajouter ? ^^

Dans ma compréhesion, toutes les fonctions dans utils*.py sont élémentaires et peuvent donc être testées.

Copy link
Member Author

Choose a reason for hiding this comment

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

J'ai rajouté 2 tests pour l'import de données du coup (utils/data_processing.py)

cy.contains("45.76")
cy.contains("4.85")
cy.contains("Plantabilité élevée")
cy.contains("Plantation favorisée")
Copy link
Contributor

Choose a reason for hiding this comment

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

Si tu utilises un enum, tu pourras directement l'utiliser ici.

@ludovicdmt
Copy link
Member Author

Il manquera juste les updates dans le changelog quand j'aurais re-généré la base..!

Copy link
Contributor

@Marc-AntoineA Marc-AntoineA left a comment

Choose a reason for hiding this comment

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

Okay, des détails en review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Top d’avoir complété

return 10


def compute_robust_normalized_indice() -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Il y a eu des modifs ici depuis ma dernière review, c’est lié aux bugs dans la génération de ce WE ? 

Copy link
Member Author

Choose a reason for hiding this comment

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

Oui et que ça mettait une vie de le faire en base directe

## 🔖 0.3.0 (2025-XX-XX) - XXX
## 🔖 0.3.0 (2025-09-04) - Mise à jour de données et ajout calque vulnérabilité à la chaleur

### 🐛 fix: Données d'occupation des sols
Copy link
Contributor

Choose a reason for hiding this comment

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

C’est plutôt une amélioration de la partie données que réellement la correction de bugs. Maintenant, tu prends en compte les ponts.

(je trouve que c’est un peu dégradant pour ton travail de dire que c’est de la correction de bug ^^)

Les routes d'API étaient définies à la main, maintenant nous utilisant une API REST à l'aide de DjangoRestFramework

&rarr; Ticket [#98](https://github.com/TelesCoop/iarbre/issues/98)
&rarr; Commit [4bfc05d](https://github.com/TelesCoop/iarbre/commit/4bfc05d0ebea7523501d9f2a49eac4284ac7bae2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Qu’est-ce que tu trouves de plus pour un commit par rapport à un lien vers une issue ou une MR ? 

L’avantage d’une MR ou d’une issue, c’est que ça a un numéro simple et tous les commentaires, les raisons détaillées du "pourquoi".

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok ça me va aussi, j'ai juste changé par uniformité avaec le reste où l'on listait les commits mais je peux supporter une différence haha

# Journal de changements

## 🔖 0.3.0 (2025-XX-XX) - XXX
## 🔖 0.3.0 (2025-09-04) - Mise à jour de données et ajout calque vulnérabilité à la chaleur
Copy link
Contributor

Choose a reason for hiding this comment

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

tu as mis l'intégralité des devs réalisés depuis la v0.2.0 ? 

Copy link
Member Author

Choose a reason for hiding this comment

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

Oui j'ai repris les merge sur dev depuis

@@ -18,7 +19,7 @@ describe("MapScorePopup", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Le score ne pourra plus être 0.81 ? Cette valeur avait justement pour objectif de vérifier que l’arrondi était bien réalisé.

Copy link
Member Author

Choose a reason for hiding this comment

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

Non du coup, je fais du seuillage dans le back pour direct renvoyer le score dans [0,2,4,6,8,10]

@ludovicdmt ludovicdmt merged commit 07b0398 into dev Apr 8, 2025
9 checks passed
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.

ETQ utilisateur.ice je veux que les places PMR ne soient pas plantables

2 participants