Dev/code review #6
Conversation
|
Dear @bertrandlalo, thanks for the hard work. Following our call, please find some feebacks in italic related to your PR in front of each section: Changement de formeRéorganisation du repoYour proposal makes sense and the reorganisation could be done later on
PEP 8 et doc-stringOk to apply Numpy docstring
PandasI find the idea of introducing Pandas dataframe very interesting to better fit to a data pipeline process. I want to evaluate pros/cons versus list/dico data structure but definitely worth to consider it
MiscNo pb to convert print to logs as well as Came case to lower
OptimisationsThere are two important changes here that we could merge asap: shell script removal for getMediaInfo as well as goproToGPx package from Raph
Nouvelles featuresCorrection du
|
|
Dear @bertrandlalo, following previous feedbacks, my recommendation to integrated smoothly first updates is for you to open a new PR with:
This is an important step as this will allow us to remove dependancies to subprocess call as well as to gopro2gpx 3rd party. Tx |
|
@cl3m3nt @AntoineBruge
C'est dommage, j'aurais bien aimé faire une team ETL... le code, c'est tellement mieux à plusieurs ! |
|
Bonjour Raphaelle,
je suis surpris du contenu de ton message, alors que je t'avais fait part
de ma volonté d'intégrer des propositions progressivement.
Si tu le souhaites, on peut en reparler afin d'évoquer la frustration que
j'ai ressentie dans ton mail.
Nous pensions justement avec Antoine te faire signe pour un nouveau besoin
ponctuel.
Qu'en penses tu ? Je peux trouver des disponibilités la semaine prochaine
pour en parler.
Clement
…On Tue, Jul 21, 2020 at 7:04 PM Raphaëlle Bertrand-Lalo < ***@***.***> wrote:
@cl3m3nt <https://github.com/cl3m3nt> @AntoineBruge
<https://github.com/AntoineBruge>
Juste un petit message, Clément, pour te dire que ce n'est pas très cool
de :
- fermer une Pull-Request sans en informer l'auteur
- prendre la quasi entièreté du code de la Pull-Request en question et
le *re-commit comme s'il était de toi*
- accepter ses propres Pull-Request... c'est presque absurde !
C'est dommage, j'aurais bien aimé faire une team ETL... *le code, c'est
tellement mieux à plusieurs !*
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACDYPVLOVYCQ373LWULDAE3R4XDB7ANCNFSM4MXEQ45A>
.
--
Clément
|
|
Hello Clément,
Sur GitHub, tu peux accepter certains bouts (commit) d’une Pull-Request
sans pour autant les re-commit. La différence se trouve dans la forme plus
que dans le fond, c’est à dire que la contribution en question est laissée
a l’auteur original.
C’est gentil de vouloir me re-integrer au projet après deux mois de
silence, mais j’ai l’impression que le travail d’équipe est compliqué.
J’étais super motivée mais je t’avoue que j’ai été déçue/frustrée par
l’accueil de mon taffe.
Bonne suite dans l’ETL, j’espère quand même que tu trouveras quelqu’un pour
reviewer le code, car je continue de penser que c’est risqué de vouloir
être seul sur une brique, surtout dans un projet bénévole où l’équipe est
censée être mouvante. Je vous laisse en discuter ensemble.
Bonne soirée !
Raphaëlle
Le mer. 22 juil. 2020 à 18:41, cl3m3nt <notifications@github.com> a écrit :
… Bonjour Raphaelle,
je suis surpris du contenu de ton message, alors que je t'avais fait part
de ma volonté d'intégrer des propositions progressivement.
Si tu le souhaites, on peut en reparler afin d'évoquer la frustration que
j'ai ressentie dans ton mail.
Nous pensions justement avec Antoine te faire signe pour un nouveau besoin
ponctuel.
Qu'en penses tu ? Je peux trouver des disponibilités la semaine prochaine
pour en parler.
Clement
On Tue, Jul 21, 2020 at 7:04 PM Raphaëlle Bertrand-Lalo <
***@***.***> wrote:
> @cl3m3nt <https://github.com/cl3m3nt> @AntoineBruge
> <https://github.com/AntoineBruge>
> Juste un petit message, Clément, pour te dire que ce n'est pas très cool
> de :
>
> - fermer une Pull-Request sans en informer l'auteur
> - prendre la quasi entièreté du code de la Pull-Request en question et
> le *re-commit comme s'il était de toi*
> - accepter ses propres Pull-Request... c'est presque absurde !
>
> C'est dommage, j'aurais bien aimé faire une team ETL... *le code, c'est
> tellement mieux à plusieurs !*
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <
#6 (comment)
>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/ACDYPVLOVYCQ373LWULDAE3R4XDB7ANCNFSM4MXEQ45A
>
> .
>
--
Clément
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFKOIPWCKP22MG5SHDDMIX3R44JBZANCNFSM4MXEQ45A>
.
|
|
Le mer. 22 juil. 2020 à 19:12, Raphaelle Bertrand <r.bertrand.lalo@gmail.com>
a écrit :
… Hello Clément,
Sur GitHub, tu peux accepter certains bouts (commit) d’une Pull-Request
sans pour autant les re-commit. La différence se trouve dans la forme plus
que dans le fond, c’est à dire que la contribution en question est laissée
a l’auteur original.
C’est gentil de vouloir me re-integrer au projet après deux mois de
silence, mais j’ai l’impression que le travail d’équipe est compliqué.
J’étais super motivée mais je t’avoue que j’ai été déçue/frustrée par
l’accueil de mon taffe.
Bonne suite dans l’ETL, j’espère quand même que tu trouveras quelqu’un
pour reviewer le code, car je continue de penser que c’est risqué de
vouloir être seul sur une brique, surtout dans un projet bénévole où
l’équipe est censée être mouvante. Je vous laisse en discuter ensemble.
Bonne soirée !
Raphaëlle
Le mer. 22 juil. 2020 à 18:41, cl3m3nt ***@***.***> a
écrit :
> Bonjour Raphaelle,
>
> je suis surpris du contenu de ton message, alors que je t'avais fait part
> de ma volonté d'intégrer des propositions progressivement.
> Si tu le souhaites, on peut en reparler afin d'évoquer la frustration que
> j'ai ressentie dans ton mail.
> Nous pensions justement avec Antoine te faire signe pour un nouveau besoin
> ponctuel.
> Qu'en penses tu ? Je peux trouver des disponibilités la semaine prochaine
> pour en parler.
>
> Clement
>
> On Tue, Jul 21, 2020 at 7:04 PM Raphaëlle Bertrand-Lalo <
> ***@***.***> wrote:
>
> > @cl3m3nt <https://github.com/cl3m3nt> @AntoineBruge
> > <https://github.com/AntoineBruge>
> > Juste un petit message, Clément, pour te dire que ce n'est pas très cool
> > de :
> >
> > - fermer une Pull-Request sans en informer l'auteur
> > - prendre la quasi entièreté du code de la Pull-Request en question et
> > le *re-commit comme s'il était de toi*
> > - accepter ses propres Pull-Request... c'est presque absurde !
> >
> > C'est dommage, j'aurais bien aimé faire une team ETL... *le code, c'est
> > tellement mieux à plusieurs !*
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <
> #6 (comment)
> >,
> > or unsubscribe
> > <
> https://github.com/notifications/unsubscribe-auth/ACDYPVLOVYCQ373LWULDAE3R4XDB7ANCNFSM4MXEQ45A
> >
> > .
> >
>
>
> --
> Clément
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#6 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AFKOIPWCKP22MG5SHDDMIX3R44JBZANCNFSM4MXEQ45A>
> .
>
|
|
Bonjour Raphaelle,
ok je comprends mieux. Si tu changes d'avis, n'hésite pas à me faire signe.
Il y a deux sujets intéressants que je pensais pouvoir te proposer.
Clement
On Wed, Jul 22, 2020 at 7:13 PM Raphaëlle Bertrand-Lalo <
notifications@github.com> wrote:
… Le mer. 22 juil. 2020 à 19:12, Raphaelle Bertrand <
***@***.***>
a écrit :
> Hello Clément,
>
> Sur GitHub, tu peux accepter certains bouts (commit) d’une Pull-Request
> sans pour autant les re-commit. La différence se trouve dans la forme
plus
> que dans le fond, c’est à dire que la contribution en question est
laissée
> a l’auteur original.
>
> C’est gentil de vouloir me re-integrer au projet après deux mois de
> silence, mais j’ai l’impression que le travail d’équipe est compliqué.
> J’étais super motivée mais je t’avoue que j’ai été déçue/frustrée par
> l’accueil de mon taffe.
>
> Bonne suite dans l’ETL, j’espère quand même que tu trouveras quelqu’un
> pour reviewer le code, car je continue de penser que c’est risqué de
> vouloir être seul sur une brique, surtout dans un projet bénévole où
> l’équipe est censée être mouvante. Je vous laisse en discuter ensemble.
>
> Bonne soirée !
>
> Raphaëlle
>
>
> Le mer. 22 juil. 2020 à 18:41, cl3m3nt ***@***.***> a
> écrit :
>
>> Bonjour Raphaelle,
>>
>> je suis surpris du contenu de ton message, alors que je t'avais fait
part
>> de ma volonté d'intégrer des propositions progressivement.
>> Si tu le souhaites, on peut en reparler afin d'évoquer la frustration
que
>> j'ai ressentie dans ton mail.
>> Nous pensions justement avec Antoine te faire signe pour un nouveau
besoin
>> ponctuel.
>> Qu'en penses tu ? Je peux trouver des disponibilités la semaine
prochaine
>> pour en parler.
>>
>> Clement
>>
>> On Tue, Jul 21, 2020 at 7:04 PM Raphaëlle Bertrand-Lalo <
>> ***@***.***> wrote:
>>
>> > @cl3m3nt <https://github.com/cl3m3nt> @AntoineBruge
>> > <https://github.com/AntoineBruge>
>> > Juste un petit message, Clément, pour te dire que ce n'est pas très
cool
>> > de :
>> >
>> > - fermer une Pull-Request sans en informer l'auteur
>> > - prendre la quasi entièreté du code de la Pull-Request en question et
>> > le *re-commit comme s'il était de toi*
>> > - accepter ses propres Pull-Request... c'est presque absurde !
>> >
>> > C'est dommage, j'aurais bien aimé faire une team ETL... *le code,
c'est
>> > tellement mieux à plusieurs !*
>> >
>> > —
>> > You are receiving this because you were mentioned.
>> > Reply to this email directly, view it on GitHub
>> > <
>>
#6 (comment)
>> >,
>> > or unsubscribe
>> > <
>>
https://github.com/notifications/unsubscribe-auth/ACDYPVLOVYCQ373LWULDAE3R4XDB7ANCNFSM4MXEQ45A
>> >
>> > .
>> >
>>
>>
>> --
>> Clément
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <
#6 (comment)
>,
>> or unsubscribe
>> <
https://github.com/notifications/unsubscribe-auth/AFKOIPWCKP22MG5SHDDMIX3R44JBZANCNFSM4MXEQ45A
>
>> .
>>
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACDYPVPULEN75QUED4NXU43R44M37ANCNFSM4MXEQ45A>
.
--
Clément
|
Hello la team ETL :) @cl3m3nt , @rcourivaud
J'ai fait pas quelques refactoring, en appliquant es corrections détaillées dans cette issue.
Pour résumer :
Changement de forme
Réorganisation du repo
J'ai essayé de faire du tri dans tous les codes, et ce que je propose comme orga du repo, c'est :
PEP 8 et doc-string
Pandas
J'utilise des DataFrame pandas, qui nous permettent d'avoir accès à pleins d'utilitaires cool. Par exemple, les données GPS ont maintenant cette tête là :
Misc
pip install git+..., pas besoin de tout cloner)make html, qui ressemble à :
[source](https://github.com/surfriderfoundationeurope/etl/tree/dev/code-review/docs/source) [build](https://github.com/surfriderfoundationeurope/etl/tree/dev/code-review/docs/build) à compléter !Optimisations
correction/optimisation de
fillGPSLa fonction
fillGPSme semblait compliquée et sujet à erreurs (par ex, si le GPS et la vidéo commencent ou finissent pas ensemble). Puis, j'ai réalisé que les seuls points qui nous intéressaient étaient en fait ceux où il y a un déchet détecté. Donc, au lieu d'interpoler toutes les données manquantes du fichier GPS, je n'interpole que les données dont le timestamp correspond à un déchet détecté par l'IA (cf mon point correction de trashGPS ci-après).changement d'ordre pour FAIL le plus tôt possible (sanity checks)
Par ex, c'est dommage de faire un call à l'IA si à la fin la connection à la DB échoue. ou si on n'arrive pas à extraire les données GPS du fichier.
J'ai enlevé le script shell qui faisait appelle à une commande juste pour avoir la durée du média, et j'ai remplacer par une fonction python.
J'ai remplacé l'appel par ligne de commande à
goproToGPxpour utiliser le paquet Pypi de Raph. D'ailleurs @rcourivaud , comment je fais pour fixer un truc sur le paquet ? (je trouve pas ton repo)J'ai implémenté un CLI pour lancer l'ETL depuis le shell. C'est peut être un peu plus pratique que le parseargs (je trouve) et on le lance avec
etl [OPTIONS]Nouvelles features
Correction du
trashGPSDu coup, ça permet d'optimiser la fonction qui prenait longtemps pour estimer les géométrie (X, Y, Z) ) partir de (latitude, longitude) car celle ci ne loop que sur les lignes où y a des déchets.
Accepte les entrées de type Smartphone vidéos et Manuelles
L'hypothèse faite ici, c'est qu'il y a 4 types de source d'acquisition possibles et que les fichiers GPX associés aux médias portent le même nom (@AntoineBruge, ok?) :
-manuelle: fichier foo.gpx
NB1: La sortie de la version manuelle est légèrement différente:
NB2: Les labels des données manuelles sont différents :
{'Objet vie courante', 'Bouteille boisson', 'Peche et chasse', 'Autre dechet', 'Industriel ou construction', 'Emballage alimentaire', 'Dechet agricole', 'Autres dechets +10'}
Pour tester ça en local, vous pouvez faire (après avoir activé l'environnement, cf README.rst) :
Pouvoir travailler 100% en local (sans se connecter à Azure & Postgre)
En fait, quand on veut tester ou développer, c'est pratique de ne pas être dépendant de Postgre et de Azure. Donc tu verras, j'ai essayé de documenter, mais dans la commande pour lancer l'ETL, on peut choisir de travailler avec des données locales plutôt que de télécharger depuis Azure (on pourra comme ça imaginer à l'avenir télécharger depuis d'autres sources). Pareil pour la sortie, on peut choisir de sauver le résultat en CSV localement plutôt que de l'insérer dans la base de données.
Todo, next steps
Qu'en dites vous ?
++