-
Notifications
You must be signed in to change notification settings - Fork 24
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
Epsg #100
Epsg #100
Conversation
myria3d/pctl/dataset/utils.py
Outdated
epsg = str(crs.to_epsg()) | ||
except Exception: | ||
raise Exception("No EPSG provided, neither in the lidar file or as parameter") | ||
|
||
return pdal.Reader.las( |
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.
Je ne suis pas sûre de comprendre pourquoi tu as choisi de faire un override_srs
même quand il y en a un dans les données. Ca me paraitrait plus logique de juste vérifier si il existe une valeur (voir si metadata['metadata']['readers.las']['srs']
existe) pour renvoyer une erreur si on n'a ni ça ni paramètre epsg
dans la config
Et ensuite d'avoir un reader sans override_srs
si on n'a pas epsg
dans la config (ça laisse à pdal la responsabilité de l'interprétation du wkt, sachant que cette fonctionnalité est utilisée par pas mal de gens dans pdal donc a priori elle n'a pas de risque de bugs)
myria3d/pctl/dataset/utils.py
Outdated
return pdal.Reader.las( | ||
filename=las_path, | ||
nosrs=True, | ||
override_srs="EPSG:2154", | ||
override_srs=f"EPSG:{epsg}", |
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.
C'est du détail, mais pour rendre lal fonctionnalité plus permissive, on pourrait donner comme paramètre srs=EPSG:2154
au lieu de epsg=2154
ce qui peut permettre d'avoir un srs custom si on ne veut pas utiliser les EPSG, sans rendre l'utilisation plus complexe
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.
On peut donner la possibilité à l'utilisateur de mettre en paramètre autre chose que EPSG (en fait je ne savais pas qu'on pouvait prendre un autre srs, mais si tu le dis ce n'est en effet pas très compliqué), cependant par cohérence il faut aussi récupérer les autres srs qui pourraient être acceptés dans les métadonnées des ficheirs lidar, donc savoir lesquels on accepte de prendre en charge et en principe il faudrait pouvoir les tester, donc avoir des ficheirs lidar avec ces srs
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.
ce que je propose c'est surtout de laisser l'utilisateur utiliser ça comme il utiliserait pdal directement (peut-être en le renvoyant vers la doc de pdal). Sauf si il y a d'autres endroits où il y a besoin de la valeur de l'epsg spécifiquement.
je ne comprends pas vraiment ce que tu entends pas "récupérer les autres srs" en fait
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.
Actuellement, il y a 2 options pour récupérer l'EPSG :
Soit on le passe en paramètre, c'est ce dont on discute
Soit on le récupère dans les métadonnées du fichier lidar
Si on peut prendre plus de srs que juste l'EPSG et que l'utilisateur choisi la seconde option, il faut récupérer le srs dans les métadonnées, pour l'instant cela ne récupère que l'EPSG, rien d'autre
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.
Ce que je propose c'est justement de ne pas gérer ça nous-même et de laisser pdal faire quand on sait qu'il y a un srs.
Ca donnerait quelque chose comme ça :
if epsg:
return pdal.Reader.las(filename, no_srs=True, override_srs=epsg)
else:
has_crs = "srs" in get_metadata(las_path)['metadata']['readers.las'].keys()
if not has_crs:
raise Exception("No EPSG provided, neither in the lidar file or as parameter")
else:
return pdal.Reader.las(filename)
On pourrait même envisager de ne même pas avoir le has_srs
et de laisser l'utilisateur avoir directement l'erreur de pdal s'il y en a une.
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.
Si on peut faire confiance à pdal on peut faire comme ça, mais pourquoi avoir voulu récupérer l'epsg dans le fichier alors ? Je pensais qu'on voulait le récupérer pour ne pas s'en remettre au comportement par défaut de pdal et imposer un comportement explicite
Si ça convient je fais comme ça
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.
Pour moi l'important c'est de pouvoir faire un override quand on donne une valeur d'epsg en paramètre de myria3d, mais ça me parait important aussi de laisser la possibilité de ne pas mettre ce paramètre et de laisser pdal se débrouiller s'il a un srs dans les données.
Ne connaissant pas le comportement de pdal en cas où il n'y a de srs ni dans le header, j'avais l'impression que c'était une bonne idée de vérifier si on avait un srs dans le header quand on ne l'a pas en paramètre pour renvoyer une erreur explicite (mais je suis moins assertive sur ce cas)
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.
j'ai mis un test concernant le champ 'srs', c'est moins direct que ce qui est suggéré car je suis tombé sur des cas particuliers. J'espère que comme c'est mis ce sera suffisant
@@ -209,7 +214,7 @@ def check_las_contains_dims(las_path: str, dims_to_check: List[str] = []): | |||
dims_to_check (List[str], optional): list of dimensions expected to be there. Defaults to []. | |||
|
|||
""" | |||
a1 = pdal_read_las_array(las_path) |
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.
J'ai l'impression que tous tes tests ont une valeur pour le srs, est-ce que tu as un test pour vérifier que ça marche si on ne le donne pas et qu'il est donné dans les métadonnées, (et un qui vérifierait que ça renvoie bien une erreur quand tu n'as pas de srs dans la donnée ni dans la config ce serait top aussi) ?
(Si tu l'as déjà ajouté qq part et que je ne l'ai pas vu, désolée pour la fausse alerte)
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.
je viens d'ajouter le test
trained_model_assets/proto151_V2.0_epoch_100_Myria3DV3.1.0_predict_config_V3.5.0.yaml
Show resolved
Hide resolved
need after all update manual for the epsg add a test for epsg
or a string describing the srs (EPSG:2154 for example)
Add the reading of the EPSG.
Now, a EPSG has to be provided, either by the lidar file or as a parameter.
To be provided by the file, it has to be in the metadata
It can also be provided as a parameter like this :
datamodule.epsg=...