-
Notifications
You must be signed in to change notification settings - Fork 4
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
maj fossil_reserves_OPEC #22
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
data-preparation/src/data_processing/FOSSIL_RESERVES_bp_fossil_with_zones_prod_2023.py
Outdated
Show resolved
Hide resolved
Bravo et merci @Lohofora pour cette pull request ! Et tout particulièrement pour la compréhension des classes et la ré-utilisation des classes CountryTranslatorFrenchToEnglish &co. J'ai fait des retours sur la PR. On est très proches de pouvoir la passer congrats 👏 |
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.
Nice job, ton code est facile à lire, bravo ! Je t'ai laissé quelques commentaires dans la PR
# Charger données OPEC -> pas API disponible | ||
# site source Maj annuelle : https://publications.opec.org/asb | ||
|
||
class BpFossilProvenReservesCleaner: |
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.
Comme pour le nom du fichier, je remplacerais Bp
par Opec
dans le nom de la classe pour éviter toute confusion dans le futur
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.
Oui tu as raison, j'avais pas capté que Bp faisait ref au nom de l'ancienne source, merci :)
def rename_column(self, df) : | ||
return df.rename(columns={'Unnamed: 0': 'country'}) | ||
|
||
def drop_unnecessary_lines(self, df) : |
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.
Est-ce que tu pourrais ajouter une docstring pour cette fonction pour expliquer quelles lignes sont inutiles ? A première vue, on se débarrasse des continents et groupements de pays, mais avec df.loc[:"Total World", :]
j'ai l'impression qu'on garde toutes les lignes jusqu'à "Total World"
. Du coup, si c'est bien le cas, pourquoi on ne garde pas les lignes qui suivent ?
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 sont soit des lignes vides ou des notes de bas de page pas utiles à notre traitement, j'ajoute la précision dans la fonction
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.
Maintenant que la source a changé, je renommerais le fichier pour éviter toute confusion dans le futur, du style FOSSIL_RESERVES_opec_fossil_with_zones_prod_2023
. Je renommerais aussi le fichier csv dans server/data
mais j'ai peur que ça impacte le front. Peut-être que pour le moment il vaudrait mieux ajouter un commentaire dans ce fichier pour préciser qu'il permet de générer FOSSIL_RESERVES_bp_fossil_with_zones_prod.csv
.
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 commenté le fichier py mais pas modifié le nom du csv pour éviter les bugs avec le front, merci :)
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 ajouté quelques commentaires de plus sur drop_unnecessary_lines
, pour le reste ça me paraît bon :)
def rename_column(self, df) : | ||
return df.rename(columns={'Unnamed: 0': 'country'}) | ||
|
||
def drop_unnecessary_lines(self, df) : |
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'ajouterais aussi un commentaire pour dire où se trouve ces lignes vides et les notes de bas de page. Et dans la docstring, j'expliquerais aussi pourquoi on exclut certains continents et certains groupements de pays, car sur le site on peut filtrer sur ce type de zones géographiques. Ça nous évitera aussi de nous reposer la question dans quelques mois :)
Ça donnerait quelque chose comme ça :
def drop_unnecessary_lines(self, df) : | |
def drop_unnecessary_lines(self, df) : | |
""" | |
Drop continents, country groups and footnotes that are not necessary for data processing | |
We exclude certain continents and country groups because ... | |
""" | |
df.set_index('country', inplace=True) | |
# After "Total World", we have empty lines or footnotes | |
df = df.loc[:"Total World", :] |
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 plus clair tu as raison merci
""" | ||
df.set_index('country', inplace=True) | ||
df = df.loc[:"Total World", :] | ||
df.drop(['Africa ', 'Latin America', 'Other Asia', 'Other Eurasia', 'Middle East ', 'OECD Europe', 'OECD Asia Pacific', 'OECD Americas', 'Others', 'Other Europe', 'Total World'], axis = 0, inplace = True) |
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 de m'en rendre compte mais il y a des espaces dans 'Africa '
et 'Middle East '
, est-ce que c'est ce qu'il y a dans les données ?
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.
Tu as raison c'est une erreur
. Ajout du code de nettoyage de la base OPEC
. Ajout du Csv avec les nouvelles données pour Oil