search

rzine-reviews / OTP_RZINE

https://rzine-reviews.github.io/OTP_RZINE/
0 stars 1 forks source link

Review #1

Open mthh opened 9 months ago

mthh commented 9 months ago

1) Avis général :

Le papier présente l'utilisation d'OpenTripPlanner depuis R et la réalisation d'analyses d'accessibilité multimodale d’un territoire (la métropole de Montpellier) à partir des données GTFS et de données OpenStreetMap.

Selon moi la fiche peut être publiée après avoir effectué les quelques corrections mineures qui sont évoquées dans les parties qui suivent. Je pense que son contenu est utile à la communauté francophone travaillant sur les questions d'accessibilité. L'article et les codes qu'il contient me paraissent facilement réutilisables pour des débutants en R.

L'article est didactique (présentation du formalisme GTFS, présentation rapide d'OSM, installation d'OTP et récupération des différentes données nécessaires) et les exemples présentées sont d'un niveau de difficulté graduel (carte leaflet, carte leaflet et légende, etc.), permettant une bonne compréhension du code par le lecteur.

La fiche mobilise les différentes fonctionnalités classiques des engins de routage (calcul d'itinéraire, calcul de matrices de temps de parcours, affichage d'isochrones) et exploite des fonctionnalités propres aux engins de routage permettant d'utiliser les transports en commun (notamment en proposant une analyse d'accessibilité sous contrainte horaire).

Le code de l'article (couplé aux données présentes sur Zenodo) fonctionne correctement. Il faut toutefois disposer de la bibliothèque prettymapr (cela n'est indiqué à aucun moment dans la fiche) pour pouvoir faire fonctionner la fonction annotation_map_tiles.


2) Retour détaillé :

Dans l'encart au dessus de l'introduction, je ne sais pas si on peut vraiment présenter OpenTripPlanner comme un "nouvel outil". Je crois qu'il est développé depuis 2009, la version 1.0 a été atteinte en 2016, et la version 2.x est développée depuis 2018.

Concernant le titre de la partie "2.3 Relations entre données GTFS et OSM.PBF" je pense qu'il pourrait être renommé pour être moins spécifique en "Relations entre données GTFS et données OpenStreetMap" (car son contenu ne s'applique pas spécifiquement au format PBF mais bien aux données OSM en général). Idem pour "Les données .osm.pbf de la Geofabrik sont mises à jour tous les jours". Geofabrik propose des données OSM sous d'autres formats (notamment au format XML, compressé avec bzip2, avec l'extension osm.bz2), qui sont elles aussi mises-à-jour quotidiennement. Si pas de changement du titre, il faudrait au moins homogénéiser la casse entre le titre 2.2.2 ("[...] données osm.pbf") et le titre 2.3 ("[...] données OSM.PBF")

Concernant l'installation d'OTP vous évoquez dans la partie 3.1 qu' "il est donc recommandé de désinstaller ces versions en amont de l’installation de Java 8 et du lancement d’OTP". Je ne sais pas si c'est un "bon" conseil car ça pourrait avoir des conséquences négatives sur la machine de l'utilisateur (il existe des solutions pour faire cohabiter sereinement plusieurs environnement d'exécution JAVA, sur Ubuntu Linux avec sudo update-alternatives --config java par exemple, sur Windows la page https://www.happycoders.eu/java/how-to-switch-multiple-java-versions-windows/ dit "Installing multiple Java versions in parallel is incredibly easy in Windows" - je ne peux malheureusement pas confirmer cette affirmation car je n'ai pas testé sur Windows).

Dans la partie 3.2, il est évoqué que la fonction otp_dl_jar télécharge "la dernière version du logiciel". Ça ne me parait pas correct car cette fonction télécharge la version 1.5 alors que la dernière version est la version 2.4.

Dans la partie 5.3, l'idée de représenter différemment les étapes du trajet selon le mode de transport est intéressante, mais le trajet sur lequel cela est effectué ne met pas beaucoup cela en valeur (il est nécessaire de beaucoup zoomer pour trouver la portion, d'une minute, de trajet piéton). Peut-être un exemple plus parlant pourrait-il être trouvé ?

Dans la partie 6.1.2, concernant les deux cartes leaflet, lors de leur premier affichage (et avant d'avoir interagis avec elles), la couche "vélo" masque la couche "transit", il est nécessaire de désactiver puis réactiver la couche "transit" pour la voir apparaître au dessus de la couche vélo. Est-il possible d'améliorer cela ou est-ce une limitation de la bibliothèque leaflet ?

Dans la partie 6.2.1 le lien vers le fichier à télécharger (data/mairies_EM.csv) ne fonctionne pas, le fichier s'appelle "mairies_3M.csv" dans votre dépôt de code.

Dans la partie 6.2.3, l'histogramme qui est produit pourrait avoir un titre plus explicite que "histogramme".

Sur la carte de la partie 6.2.4, il aurait pu être utile de faire figurer la localisation de la gare Saint Roch (d'autant que les symboles proportionnels semblent eux être localisés à l'endroit de la mairie plutôt qu'au centroide de la commune, c'est qui est une bonne chose je pense, mais qui renforce l'envie d'avoir la localisation de la gare Saint Roch)


3) Concernant la forme - extraits de code

Il faudrait éventuellement harmoniser la manière d'écrire les noms de variables (par endroit c'est en snake_case, path_otp par ex., et à d'autres en camelCase, myOtpcon par ex.). Également Mairies -> mairies.

Par ailleurs, dans la majorité des cas, lors de l'écriture des arguments d'une fonction, vous mettez un espace après la virgule, sauf dans de très rares cas (select(fromPlace,toPlace,duration) par exemple - également distinct(fromPlace,route_option, .keep_all = TRUE) où les 2 sont mélangés). À harmoniser selon moi pour une meilleur lisibilité du code (toujours mettre l'espace).

Idem pour les espaces autour du symbole = , par exemple dans l'instruction :

route_3 <- otp_plan(otpcon=myOtpcon, 
                  fromPlace = start_places, 
                  toPlace = end_places,
                  mode = c("TRANSIT","WALK"),
                  date_time = as.POSIXct("2023-04-19 10:15:00"),
                  get_geometry=FALSE)

Également à harmoniser selon moi (toujours mettre les espaces).

Ces différents éléments de style me poussent à conseiller aux auteurs l'utilisation d'un linter ainsi qu'à conseiller à la revue Rzine la définition d'un guide de style + recommander l'utilisation d'un linter (ça me parait intéressant pour une revue qui se destine à accueillir du code dans chacun de ces articles).

Pour pinailler : route_boucle pourrait être renommé en routes (ça n'apporte rien de préciser, dans le nom de variable, qu'on remplit ce data.frame en faisant une boucle).

Enfin, il pourrait être judicieux de renommer certaines variables afin qu'elles soient toutes définies dans la même langue (on a par exemple temps_limite et temps_debut puis dans le bloc de code suivant end_places et from_places).


4) Concernant la forme - reformulations et typos

Quelques erreurs que j'ai pu relever lors de ma lecture :

De plus, à titre personnel, en français je pense qu'il faut privilégier l'usage de "bibliothèque" par rapport à l'utilisation de l'anglicisme "librairie" (mais je ne sais pas quelles sont les conventions de Rzine à ce sujet).

Dans la conclusion, "Nous espérons que cette fiche RZINE vous aura aidés à vous en approprier les fonctionnalités et potentialités." ne me parait pas forcément utile (et sinon, "aidés" -> "aidé" il me semble).

jc-ulles commented 9 months ago

Bonjour @mthh, nous vous remercions pour votre expertise sur notre proposition de fiche Rzine. Vous mentionnez que la fonction annotation_map_tile ne s'est pas correctement activée dans la version que nous proposons. Pourtant, cette fonction est issue de la bibliothèque ggspatial (et non prettymapr d'après nos recherches). Lors de l'édition de la fiche, nous n'avons pas rencontré de problèmes. Est-il possible d'avoir le message d'erreur en détail pour mieux cibler le souci de notre côté ? Je vous remercie.

mthh commented 9 months ago

Message d'erreur : 

Error in `annotation_map_tile()`:
! Problem while converting geom to grob.
ℹ Error occurred in the 1st layer.
Caused by error in `loadNamespace()`:
! aucun package nommé ‘prettymapr’ n'est trouvé
Run `rlang::last_trace()` to see where the error occurred.

Résultat de rlang::last_trace(drop=FALSE) : 

> rlang::last_trace(drop = FALSE)
<error/rlang_error>
Error in `annotation_map_tile()`:
! Problem while converting geom to grob.
ℹ Error occurred in the 1st layer.
Caused by error in `loadNamespace()`:
! aucun package nommé ‘prettymapr’ n'est trouvé
---
Backtrace:
     ▆
  1. ├─base (local) `<fn>`(x)
  2. ├─ggplot2:::print.ggplot(x)
  3. │ ├─ggplot2::ggplot_gtable(data)
  4. │ └─ggplot2:::ggplot_gtable.ggplot_built(data)
  5. │   └─ggplot2:::by_layer(...)
  6. │     ├─rlang::try_fetch(...)
  7. │     │ ├─base::tryCatch(...)
  8. │     │ │ └─base (local) tryCatchList(expr, classes, parentenv, handlers)
  9. │     │ │   └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 10. │     │ │     └─base (local) doTryCatch(return(expr), name, parentenv, handler)
 11. │     │ └─base::withCallingHandlers(...)
 12. │     └─ggplot2 (local) f(l = layers[[i]], d = data[[i]])
 13. │       └─l$draw_geom(d, layout)
 14. │         └─ggplot2 (local) draw_geom(..., self = self)
 15. │           └─self$geom$draw_layer(...)
 16. │             └─ggplot2 (local) draw_layer(..., self = self)
 17. │               └─base::lapply(...)
 18. │                 └─ggplot2 (local) FUN(X[[i]], ...)
 19. │                   ├─rlang::inject(self$draw_panel(data, panel_params, coord, !!!params))
 20. │                   └─self$draw_panel(...)
 21. │                     └─ggspatial (local) draw_panel(...)
 22. │                       └─ggspatial:::project_extent(...)
 23. ├─base::loadNamespace(x)
 24. │ ├─base::withRestarts(stop(cond), retry_loadNamespace = function() NULL)
 25. │ │ └─base (local) withOneRestart(expr, restarts[[1L]])
 26. │ │   └─base (local) doWithOneRestart(return(expr), restart)
 27. │ └─base::stop(cond)
 28. └─rlang (local) `<fn>`(`<pckgNtFE>`)
 29.   └─handlers[[1L]](cnd)
 30.     └─cli::cli_abort(...)
 31.       └─rlang::abort(...)

On peut donc remonter jusqu'à la fonction ggspatial::project_extent. Si on regarde dans le code de cette fonction on peut voir quelle appelle une fonction de prettymapr dans certains cas (d'ailleurs sur https://cran.r-project.org/web/packages/ggspatial/index.html on peut voir que prettymapr est dans le champ suggests).

jc-ulles commented 9 months ago

Merci, nous allons le prendre en compte.

jc-ulles commented 9 months ago

Cher évaluateur, Nous vous remercions pour l'avis favorable porté sur notre proposition de fiche Rzine et pour les propositions d'amélioration que vous nous avez transmises. Nous avons tenu compte de l'ensemble d'entre elles, et vous soumettons en conséquence une version révisée de la fiche. Les codes sont aa4a3a0a02db249186385a70ec99fbf41812ce1a et ff1052e30dc74745b7dc7c5c07ae95616738e886. Nous restons à votre disposition en cas de besoin. Cordialement, Jean-Clément Ullès et Marion Le Texier

mthh commented 6 months ago

Mes excuses pour le délai de réponse.

Vos corrections me semblent répondre à ma review initiale :+1: