Skip to content
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

add ports to db #104

Closed
wants to merge 1 commit into from
Closed

add ports to db #104

wants to merge 1 commit into from

Conversation

romainm13
Copy link
Collaborator

  • Adding ports to db
  • Adding some processing utils functions in /data
  • ask Bloom for ports.csv or ports_rad3000_res10.csv

New alembic history is

(bloom-py3.10) *[main][~/12_bloom]$ alembic history
961cee5426d6 -> 7962eee40abe (head), create ports table
1fd83d22bd1e -> 961cee5426d6, create amp table
68c9f220a07f -> 1fd83d22bd1e, create alert table
e52b9542531c -> 68c9f220a07f, add columns in spire table
<base> -> e52b9542531c, create marine traffic positions and polygons table

To load data do make load-ports-data just after make launch-dev-db

@RonanMorgan
Copy link
Collaborator

pour moi c'est ok .
@rv2931 @njouanin ok pour vous ?

@rv2931
Copy link
Collaborator

rv2931 commented Mar 12, 2024

Il y a le docker-compose-load-data.yaml qu'il faudrait mettre a jour pour ajouter le load_ports_data.py
Mais faudrait le rebase non ?

@njouanin
Copy link
Collaborator

Je veux bien jeter un œil (ce soir) avant de merger

"""
op.create_table(
"ports",
sa.Column("id", sa.Integer, primary_key=True),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sa.Column("id", sa.Integer, sa.Identity(), primary_key=True, index=True),

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

à quoi servent le sa.Identity() ? et index=True ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ça permet d'indiquer à SqlAlchemy d'utiliser la syntaxe GENERATED ALWAYS AS IDENTITY pour le type de la PK. Voir ici: https://www.postgresqltutorial.com/postgresql-tutorial/postgresql-identity-column/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Index = True, effectivement pas utile pour une PK, c'est implicite

from shapely import wkt
from shapely.geometry import Polygon

radius_m = 3000 # Radius in kilometers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est pas un peu beaucoup 3000km ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est en metres je me suis trompé dans le commentaire. Le code est bon

Fix: radius_m = 3000 # Radius in meters

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu peux corriger ?

@rv2931
Copy link
Collaborator

rv2931 commented Mar 13, 2024

@njouanin @romainm13
Est-ce que la méthode d'intégration/stockage des données ports correspond au modèle qui est en train d'être défini. @njouanin c'est bien toi qui est responsable de la structure BDD on est d'accord ?
Si oui tu pourrais mettre le lien vers le notion/miro qui en parle afin qu'on puisse vérifier que c'est bien cohérent avec l'archi cible ?

@rv2931
Copy link
Collaborator

rv2931 commented Mar 13, 2024

Pour pas d’ambiguïté et risque de merge un peu rapide, je propose de passer la PR en draft le temps qu'on soit ok pour valider

@rv2931 rv2931 marked this pull request as draft March 13, 2024 14:43
@njouanin
Copy link
Collaborator

njouanin commented Mar 13, 2024

@rv2931 c'est bien moi qui suis responsable de la structure de la BDD. Ce qu'a fait @romainm13 est proche du modèle cible. La référence est en haut du miro, sachant que le modèle est encore en brouillon.
Je propose de merger cette PR et je reprends ensuite le code pour le mettre au carré.

@rv2931 rv2931 marked this pull request as ready for review March 13, 2024 21:16
from shapely import wkt
from shapely.geometry import Polygon

radius_m = 3000 # Radius in kilometers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu peux corriger ?

@rv2931
Copy link
Collaborator

rv2931 commented Mar 13, 2024

Bon c'est du détail globalement mais il y a quelques exceptions ruff sur de la qualité de code
ça a pas l'air compliqué mais si on peut éviter des les cumuler c'est pas plus mal
Faut aller regarder les Check/Pre-Commit/Run Pre-commit

@njouanin
Copy link
Collaborator

Bon c'est du détail globalement mais il y a quelques exceptions ruff sur de la qualité de code ça a pas l'air compliqué mais si on peut éviter des les cumuler c'est pas plus mal Faut aller regarder les Check/Pre-Commit/Run Pre-commit

oui, je pensais ouvrir une issue spécifique pour que l'on fasse une passe de correction ruff globale sur le code.

@rv2931
Copy link
Collaborator

rv2931 commented Mar 15, 2024

@romainm13 j'ai créé une branche add_ports à partir de ta branche de ton fork
Je l'ai rebase sur le nouveau main et mis à jour le docker compose load data pour y intégrer le chargement des ports et quelques modification
Par contre j'ai une erreur quand je fais la commande alembic upgrade head
ERROR [alembic.util.messaging] Multiple head revisions are present for given argument 'head'; please specify a specific target revision, '<branchname>@head' to narrow to a specific head, or 'heads' for all heads FAILED: Multiple head revisions are present for given argument 'head'; please specify a specific target revision, '<branchname>@head' to narrow to a specific head, or 'heads' for all heads

J'imagine qu'il y a une divergence de 2 branches en Y de ce que je comprends. Si vous maitrisez alembic je vous laisse résoudre sinon j'imagine qu'il faut juste refaire la cohérence down_revision et depends_on ?

@rv2931 rv2931 mentioned this pull request Mar 15, 2024
@njouanin
Copy link
Collaborator

Si tu lances alembic history tu n'as pas la même chose que @romainm13 dans son premier commentaire ?

@rv2931
Copy link
Collaborator

rv2931 commented Mar 15, 2024

Je vous propose de repartir de cette PR #111
La branche est créé dans le dépôt principal et on peut collaborer directement dessus

@rv2931
Copy link
Collaborator

rv2931 commented Mar 15, 2024

Si tu lances alembic history tu n'as pas la même chose que @romainm13 dans son premier commentaire ?

J'ai modifié un down_revision et j'obtiens ça qui fonctionne

6734d9afbba5 -> 7962eee40abe (head), create ports table
961cee5426d6 -> 6734d9afbba5, drop_marine_traffic_vessel_positions
1fd83d22bd1e -> 961cee5426d6, create amp table
68c9f220a07f -> 1fd83d22bd1e, create alert table
e52b9542531c -> 68c9f220a07f, add columns in spire table
<base> -> e52b9542531c, create marine traffic positions and polygons table

On a du intégrer des revisions pour marine traffic entre temps

@rv2931
Copy link
Collaborator

rv2931 commented Mar 16, 2024

code repris dans le projet via la la PR #111
Je ferme cette PR

@rv2931 rv2931 closed this Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants