Module d'identification de la végétation et des non-classés #62
Conversation
… evaluating the IoU
There was a problem hiding this comment.
Bonne PR, je ne vois pas de soucis de logique, et surtout des changements à apporter sur la cohérence de style / l'intégration avec les autres modules déjà existants.
Par ailleurs, pourras-tu à la fin vérifier que:
- la documentation se créer sans soucis de formatage pour les nouvelles fonctions
- la logique du nouveau module est elle même documentée quelque part
- tout se lance bien en ligne de commande
- notamment, le package peut être installé avec pip install -e (pour figer une version) et ensuite lancé en tant que module.
- Actualiser éventuellement la doc si il y a des changements d'API !
There was a problem hiding this comment.
OK, la structure prend vraiment une bonne forme. :) Est-ce que tu pourras jeter un coup d'oeil aux quelques commentaires non résolus encore stp ? J'en ai rajouté quelques nouveaux, généralement des détails.
Et ensuite il faudra se pencher sur la documentation, je n'ai pas vu passer de commits là dessus. :) Merci !
| - "${data_format.las_dimensions.ai_vegetation_unclassified_groups}=uint" | ||
| vegetation_unclassified_detection: | ||
| # Extra dims added for storing the result of the vegetation/unclassified detection. | ||
| - "${data_format.las_dimensions.ai_building_identified}=uint" |
There was a problem hiding this comment.
je crois que certaines dimensions sont encore supprimées à tort, en particulier les inputs de vegetation_unclassified
En gros on veut :
vegetation_unclassified :
input : p_vegetation, p_building, entropy
output : ai_vegetation_unclassified_groups, p_building, entropy
building :
input ; p_building, entropy ;
output : ai_building_identified
There was a problem hiding this comment.
Alors pour le building, il me semblait qu'il fallait aussi en sortie l'entropie (en tout cas c'était comme ça avant)
Pour la végétation, on ne passe plus dans la suppression des dimensions au début, donc pas besoin de préciser lesquelles on veut garder
Mais en effet il y a eu une inversion (j'ai confondu les proba de building avec un autre)
There was a problem hiding this comment.
Ah ok, je comprends. Donc l'input de vegetation_unclassified est bon.
Par contre dans l'output les probas sont demandées en uint et non plus en float. Ce n'est pas souhaité : ce sont des valeurs entre 0 et 1 et on veux bien des floats.
Pour building_output, j'avais probablement laissé l'entropie à titre indicatif, car c'est une info intéressante pour comprendre les erreurs éventuelles du modèle. Tu peux peut l'enlever maintenant pour alléger. :)
There was a problem hiding this comment.
Je peux enlever entropie évidemment, mais ne serait-il pas mieux de ne pas mettre cette colone lorsqu'on est en production ? J'ai remarqué que les opération d'ajout et de suppression de colonnes sont vraiment longues.
A titre d'exemple et pour ce que j'ai en tête, une opération de détection de végétation/non classé avec pdal et suppresion/ajout de colonne prenait 80-100 sec sur un fichier donné, pour le même fichier avec laspy sans opération sur les colonnes c'était 6-8 sec.
Maintenant, c'est peut-être parce que je suis limité en mémoire sur mon PC.
Il faudrait peut-être faire un schéma clair avec quelles colonnes exactement en entrée et en sortie de chaque process (myria3D, détection végé/non classé, détection building), pour éviter au maximum ces opérations qui m'ont semblées couteuses
There was a problem hiding this comment.
Je peux enlever entropie évidemment, mais ne serait-il pas mieux de ne pas mettre cette colone lorsqu'on est en production ?
On a besoin d'entropie pour building validation, et on n'en n'a plus besoin ensuite, donc oui on peut l'enlever (on veut éviter de la garder car ça alourdira les fichiers suivants sans raison).
A titre d'exemple et pour ce que j'ai en tête, une opération de détection de végétation/non classé avec pdal et suppresion/ajout de colonne prenait 80-100 sec sur un fichier donné, pour le même fichier avec laspy sans opération sur les colonnes c'était 6-8 sec.
Ce qui est le plus long avec les Cleaner c'est qu'ils nécessitent une lecture + écriture à répétition (n * 10-20secondes). Mais avec les changements que tu apporte il n'y aura pas de surcout à priori.
Il faudrait peut-être faire un schéma clair avec quelles colonnes exactement en entrée et en sortie de chaque process (myria3D, détection végé/non classé, détection building).
Je suis d'accord, ça aiderait vraiment à comprendre les choses, d'autant que le système se complexifie avec ce nouveau module. Est-ce que tu pourrais l'ajouter à cette PR ? Peut-être dans la page de doc du processus ?
Il y a http://mermaid-js.github.io/mermaid/#/ qui est adapté pour des schémas en markdown. Je ne sais pas à quel point ça peut être intégré direct dans la doc.
| destination_path = tempfile.NamedTemporaryFile().name | ||
| detect_vegetation_unclassified( | ||
| vegetation_unclassifed_hydra_cfg, | ||
| LAS_SUBSET_FILE_VEGETATION, |
There was a problem hiding this comment.
Si c'est simplement de la description je veux bien une docstring :)
CharlesGaydon
changed the title
There was a problem hiding this comment.
On est pas mal, il reste simplement de la documentation à rajouter, et un data_format à changer, ainsi que le nom de la task apply_on_building à changer vers "apply_building_module" par exemple (cf. un autre commentaire).
Par ailleurs, est-ce que tu peux vérifier que les changement d'API pour e.g. l'optimization des seuils de validation bati est bien répercuté dans toutes les pages qui le mentionnent ? Par exemple sur cette page, je crois que le nom de la tâche est modifé.
There was a problem hiding this comment.
Alors, je vois que pdal est maintenant souvent mobilisé, et c'est bien.
Cependant ces modifications n'ont pas l'effet escompté sur les écritures/lectures.
La syntaxe permettant d'éviter les écritures / lectures est donnée dans le README de python-pdal à cette section.
En gros :
points_of_first_pipeline = pipeline1.arrays[0]
pipeline2 = pdal.Filters.X().pipeline(out_of_first_pipeline)
pipeline2 = pipeline2 | ... | ... # other operations
pipeline2.execute()
points_of_second_pipeline = pipeline2.arrays[0]
L'idée étant alors de se transmettre des np.ndarray entre les différentes tasks, sans les écrire/lire.
| @@ -1,6 +1,6 @@ | |||
| # parameters | |||
| vegetation_threshold: 0.5 | |||
| unclassified_threshold: 0.5 | |||
| vegetation_threshold: 0.3923 | |||
There was a problem hiding this comment.
Est-ce que l'optimisation apporte un changement important par rapport aux seuils par défaut de 0.5 ? Par important j'entends au doigt mouillé une IoU qui s'élève de + de 1% après optimisation.
Car sinon, on peut tout aussi bien garder des seuils à 0.5. Ca permet d'éviter un overfitting éventuel sur les données d'optimisation.
There was a problem hiding this comment.
De tête il me semble que c'est un chouia plus que 1 pourcent, je n'ai pas les valeurs pour 0.5 sous la main mais c'est ce dont je me souviens pour les valeurs des "trials" intermédiaires qui étaient affichés. Sans vraiment de garantie
Le souci c'est plus que ce sont deux valeurs en-dessous de 0.5, donc en théorie les deux seuils peuvent être validé ensemble
There was a problem hiding this comment.
De tête il me semble que c'est un chouia plus que 1 pourcent, je n'ai pas les valeurs pour 0.5 sous la main mais c'est ce dont je me souviens pour les valeurs des "trials" intermédiaires qui étaient affichés. Sans vraiment de garantie
OK. On peut garder pour l'instant. Lorsqu'on aura la nouvelle version du modèle sur données à la végétation corrigée, il faudra évaluer proprement les IoU, et enrichir la ModuleCard qui contient pour l'instant une évaluation du module bati seul.
Le souci c'est plus que ce sont deux valeurs en-dessous de 0.5, donc en théorie les deux seuils peuvent être validé ensemble
Ca peut être une bonne raison d'exiger un seuil plus élevé.
There was a problem hiding this comment.
Revu ; quelques typos, mais surtout : l'objectif de revoir les entrées/sorties est de diminuer les lectures et écritures. Je crois que tu as diminué les lectures grâce au passage de l'objet pipeline à travers les objets.
Par contre les écritures sont quasi-systématiques encore dans les différents sous-modules du bâti.
A mon avis on peut avoir une signature générale pour tous du type :
func(input_values: str | Pipeline, target_las_path:Optionnal[str] = None)`:
self.pipeline = get_pipeline(input_values)
Et préciser target_las_path permet d'activer la sauvagarde.
There was a problem hiding this comment.
Une mini typo.
Tu as pu vérifier la doc ?
Sinon LGTM.
Tu peux alors :
- Passer le numéro de version à 1.7.0 dans package_metadata (cf. un de mes coms non résolus),
- Rebase sur main pour prendre en compte le confilt sur package_metadata
- squash & merge cettez PR. :)
docs/source/background/overview.md
Outdated
| @@ -2,8 +2,8 @@ | |||
|
|
|||
| A las file goes through several steps, which alter its content. | |||
| At first we have a `Raw` las file, sent to another process (`Myria3D`) that infers the probabilities of various classes. | |||
| From there, the las file is used a first time by this module to decide if the points are vegetation, unclassified or something else, and sent to an exterior process, a `rule based segmentation`. | |||
| The las file is used a second time by this module to decide if the points are buldings or not. | |||
| From there, the las file is used a first time by this module to decide if the points are vegetation, unclassified or something else, and sent to an exterinal process, a `rule-based segmentation`. | |||
There was a problem hiding this comment.
exterinal -> external
;)
| self.pipeline = pdal.Pipeline(arrays=[las]) | ||
|
|
||
| if target_las_path and type(target_las_path) == str: | ||
| if target_las_path: |
No description provided.