mobility-team / mobility

Mobility, an open-source library for mobility modelisation
MIT License
16 stars 10 forks source link

Add db comparison example #40

Closed louisegontier closed 1 year ago

louisegontier commented 1 year ago

Add a script to compare ENTD 2008 and EMP 2019 database (in /examples) Linked with article for the future project's website

codecov[bot] commented 1 year ago

Codecov Report

Merging #40 (d8360bc) into main (c71eb27) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #40   +/-   ##
=======================================
  Coverage   94.76%   94.76%           
=======================================
  Files          10       10           
  Lines         497      497           
=======================================
  Hits          471      471           
  Misses         26       26           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Mind-the-Cap commented 1 year ago

Merci @louisegontier pour cette PR, ce sera très utile d'avoir ces comparaisons.

Plusieurs points techniques de mon côté :

La distance moyenne dans la bdd EMP-2019 est de 480 km. La distance moyenne dans la bdd ENTD-2008 est de 379 km. La distance moyenne dans la bdd EMP-2019 est de 480 km. La distance moyenne dans la bdd ENTD-2008 est de 379 km. La distance moyenne dans la bdd EMP-2019 est de 480 km. La distance moyenne dans la bdd ENTD-2008 est de 379 km. La distance moyenne dans la bdd EMP-2019 est de 484 km. La distance moyenne dans la bdd ENTD-2008 est de 379 km. La distance moyenne dans la bdd EMP-2019 est de 548 km. -avec pondération La distance moyenne dans la bdd ENTD-2008 est de 354 km. -avec pondération La distance moyenne dans la bdd EMP-2019 est de 548 km. -avec pondération La distance moyenne dans la bdd ENTD-2008 est de 354 km. -avec pondération La distance moyenne dans la bdd EMP-2019 est de 548 km. -avec pondération La distance moyenne dans la bdd ENTD-2008 est de 355 km. -avec pondération La distance moyenne dans la bdd EMP-2019 est de 550 km. -avec pondération La distance moyenne dans la bdd ENTD-2008 est de 354 km. -avec pondération

Cela me semble un peu répétitif. Est-ce que ça te paraît normal ?

Point de détail : le code ne suit pas PEP8, est-ce qu'il te serait possible de passer un coup de black ou autre solution de ton choix ?

Merci beaucoup !

louisegontier commented 1 year ago

Merci pour ta relecture !

louisegontier commented 1 year ago

J'ai ajouté un nouveau commit pour fix ces 3 points. https://github.com/mobility-team/mobility/pull/40/commits/661b88da3f9cbf45107f80059ca69f3a3e6d6a01 Pour les figures, j'ai enlevé le renderer="png", maintenant dans pycharm mes plots s'affichent mais dans des pages web (attention il y a beaucoup de graphs quand on lance toutes les fonctions). Dis moi si ça fonctionne pour toi aussi @Mind-the-Cap Pour le format, j'ai fait dans pycharm :)

Mind-the-Cap commented 1 year ago

Merci beaucoup @louisegontier ! Après beaucoup d'errements j'ai réussi à faire marcher ça avec fig.show(renderer="browser") (dans Spyder, c'est celui que j'utilise). J'ai donc rajouté avertissements et conseils en commentaire, avec quelques autres petites améliorations. Je te laisse l'honneur de merger si c'est bon pour toi :)

louisegontier commented 1 year ago

Super, merci beaucoup pour tes tests et pour l'astuce!

Une dernière question avant de merger, pour ce test j'utilise des fichiers excel de correspondance (pour les modes et les motifs : fichiers entd_mode.xlsx et entd_location_motive_group.xlsx) qui pourraient être utiles sûrement ailleurs dans l'outil => faut-il les déplacer à un autre endroit de l'arborescence ? J'utilise également le fichier mode_ef.csv pour la partie où l'on estime les émissions carbone associées aux déplacements qui est redondante avec la future fonction carbon_computation => est-ce que l'on conserve cette version, on merge, on merge ensuite la fonction carbon et on met à jour à la fin ou on part du principe que la fonction carbon existera et on l'implémente dès maintenant ? N'hésitez pas à me dire si mon message n'est pas clair du tout 😄

Mind-the-Cap commented 1 year ago

Je pense que pour l'instant on peut merger et ouvrir une issue pour se rappeler de :