feat : Added new markers types and live alerts features #23
Conversation
incorporating pyronear logo, black colored background, new cta style and a centered text retreiving username logged
brought both alerts and risks dashboards onto hompage, adding a filter box gathering existing filters and adding some. Meteo graphs are hidden but can be displayed if desire
…nput to new live alert components
Akilditu
added
type: enhancement
There was a problem hiding this comment.
Nice job @Akilditu, thanks!
I just added some comments on the form rather than the content. This is a really cool intermediate stage from which we'll be able to iterate and implement some of our UX specialists' recommendations :)
My only question: do we know how the alert_radio_button will be linked to the API?
Moreover, at this point, the build_live_alerts_metadata() function stores a single alert information, I guess we'll have to take into account the 'multiple alerts at once' scenario, though we can deal with it later.
There was a problem hiding this comment.
@Akilditu thanks for this great job! 💪
The pieces you added to our first commits looks consistent with what we did and I really the overall workflow that is very smooth :)
Remaining action will be to replace build_live_alerts_metadata() by real API data and store it in cache.
There was a problem hiding this comment.
Thanks a lot for this great PR @Akilditu!
I have left a number of comments but these are either typos / very small suggestions or more long-term issues with which we probably don't want to deal at the moment.
The only question that might be somewhat substantial is related to the loading of alert_metadata: in main.py, we make a first call at the build_live_alerts_metadata function, so that we can load the data only once. But in other files, all functions related to alerts also call the build_live_alerts_metadata function on their own if I am not mistaken. Shouldn't they instead take the alert_metadata object that has been instantiated as an argument?
EDIT: Oops, I had not seen Vincent's comment above mine, especially the part about alert_metadata and storing it in cache; feel free to ignore my remark if I have just missed the discussion on this topic.
|
|
||
| # We read the csv file that locates the cameras and filter for the department of interest | ||
| camera_positions = pd.read_csv(Path(__file__).parent.joinpath('data', 'cameras.csv'), ';') | ||
| camera_positions = camera_positions[camera_positions['Département'] == int(dpt_code)].copy() |
There was a problem hiding this comment.
Thanks a lot for the renaming from build_alerts_geojson to build_departments_geojson or from get_camera_positions to build_sites_markers, it makes the code much clearer! In the same vein, shouldn't we change camera_positions into site_positions?
| alert_codis=alert_codis, | ||
| nb_device=nb_device, | ||
| popup=popup)) | ||
| markers.append(dl.Marker(id=f'site_{i}', # Necessary to set an id for each marker to reteive callbacks |
There was a problem hiding this comment.
| markers.append(dl.Marker(id=f'site_{i}', # Necessary to set an id for each marker to reteive callbacks | |
| markers.append(dl.Marker(id=f'site_{i}', # Necessary to set an id for each marker to receive callbacks |
Small typo in the comment.
Just to confirm and if I am not mistaken, the site table that we will call via the API contains a site_id which we will be able to use instead of the index of the for loop here, right?
There was a problem hiding this comment.
yes I think you're right @pechouc, the f-string id was the previous way of getting a marker, from now on we should have a real site_id
| nb_device=nb_device, | ||
| popup=popup)) | ||
| markers.append(dl.Marker(id=f'site_{i}', # Necessary to set an id for each marker to reteive callbacks | ||
| position=(lat, lon), |
There was a problem hiding this comment.
Probably not a priority for the moment but I wonder whether it is a good practice to define an extensive set of variables which we use only once, a few lines below (like lat, lon, site_name...). But clearly, we can move forward with this until demo day, as variable names are explicit.
| html.P(f'Coordonnées : ({lat}, {lon})'), | ||
| html.P(f'Nombre de caméras : {nb_device}')])])) | ||
|
|
||
| # We group all dl.Marker objects in a dl.LayerGroup object |
There was a problem hiding this comment.
| # We group all dl.Marker objects in a dl.LayerGroup object | |
| # We group all dl.Marker objects in a dl.MarkerClusterGroup object |
I guess this has changed since the comment was written down!
app/alerts.py
Outdated
| html.P(f'Nombre de caméras : {nb_device}')])])) | ||
|
|
||
| # We group all dl.Marker objects in a dl.LayerGroup object | ||
| markers_layer = dl.MarkerClusterGroup(children=markers, id='sites_markers') |
There was a problem hiding this comment.
Maybe we can call it markers_cluster instead of markers_layer as the type of object seems to have changed?
app/alerts.py
Outdated
| # "popupAnchor": [-3, -76] # Point from which the popup should open relative to the iconAnchor | ||
| } | ||
| alerts_markers = [dl.Marker( | ||
| id="marker_{}".format(alert_id), # Setting a unique id for each alerts_markers |
There was a problem hiding this comment.
| id="marker_{}".format(alert_id), # Setting a unique id for each alerts_markers | |
| id="alert_marker_{}".format(alert_id), # Setting a unique id for each alerts_markers |
To make it more explicit in case we want to call that id in a call-back at some point?
|
|
||
| # Defining center and zoom parameters for map_object | ||
| if n_clicks > 0: | ||
| center = [alert_lat, alert_lon] |
There was a problem hiding this comment.
I really like the fact that everything is already variabilized outside of the build_live_alerts_metadata function 😄
app/homepage.py
Outdated
|
|
||
| # Fetching alert status and reusable metadata | ||
| alert_metadata = build_live_alerts_metadata() | ||
| alert_lat = (alert_metadata["lat"]) |
There was a problem hiding this comment.
Is there any use to the parentheses that frame alert_metadata["lat"]? 🧐
app/homepage.py
Outdated
| @@ -80,52 +162,57 @@ def meteo_graphs(display=False): | |||
| # Content and App layout | |||
| # The following function is used in the main.py file to instantiate the layout of the homepage | |||
|
|
|||
| # Body container | |||
There was a problem hiding this comment.
Isn't this part a duplicate of what is inside the Homepage function? Sorry if I am missing something obvious, but is there any reason we should keep it?
| @@ -80,36 +79,49 @@ def display_page(pathname): | |||
| # ------------------------------------------------------------------------------ | |||
| # Callbacks related to the "Alertes et Infrastructures" dashboard | |||
|
|
|||
| # Fetching reusable alert metadata | |||
| alert_metadata = build_live_alerts_metadata() | |||
There was a problem hiding this comment.
Isn't this contradictory with the fact that we are calling the build_live_alerts_metadata function in each of the functions that are related to alerts in the other files (notably build_alerts_elements or build_alerts_markers in alerts.py and display_alerts_frames in homepage.py)? Shouldn't these functions take an alert_metadata as argument so that we don't need to create it each time we call these functions?
Indeed this is the next PR purpose : fetching API data for real ! |
It will be deleted and the function build_alerts_elements() will be triggered by the API response
I was thinking about iterating through the API response DataFrame for that purpose |
Good idea, done ! 👍 |
feat : Added new markers types and live alerts features (pyronear#23)
This PR aims at adding new components, methods and callbacks to handle live alerts and markers uses better.
Fixes #14
Here is a snapshot of how the whole app renders depending on both
no alertandalertstates that can be set by radio items:Few details because discussed a lot before among issues and meetings but I remain of course available for any matter.