Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical runtime and energy visualizer correctness issues block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds two Dash demo applications: a lithium supply-chain Sankey dashboard and an energy network visualizer using Cytoscape and Leaflet.
Changes:
- Adds lithium data utilities, Sankey visualization, filters, sample data, styling, tests, and deployment configuration.
- Adds synthetic energy-network data, map visualization, topology editing, load charts, and dependencies.
File summaries
| File | Summary |
|---|---|
lithium-supply-chain/utils/ui_utils.py |
Sankey controls and node-card UI. Nits: rename the misspelled callback and fix the comment apostrophe. |
lithium-supply-chain/utils/data_utils.py |
Data loading and dropdown helpers. |
lithium-supply-chain/utils/chart_utils.py |
Sankey data and figure generation. Moderate: correct the six-value return annotation and documentation. Nits: fix documented spelling and wording errors. |
lithium-supply-chain/utils/__init__.py |
Utility module exports. |
lithium-supply-chain/tests/test_ui_utils.py |
UI test scaffold. |
lithium-supply-chain/tests/test_data_utils.py |
Data utility tests. |
lithium-supply-chain/tests/test_chart_utils.py |
Moderate: compare sorted lists directly; the current assertion always passes. |
lithium-supply-chain/tests/mock_data.py |
Test fixtures. |
lithium-supply-chain/tests/__init__.py |
Test package exports. |
lithium-supply-chain/runtime.txt |
Critical: declared Python 3.8.3 is incompatible with the pinned Werkzeug version. |
lithium-supply-chain/requirements.txt |
Lithium app dependencies. |
lithium-supply-chain/Procfile |
Gunicorn entry point. |
lithium-supply-chain/pages/supply_sankey.py |
Moderate: apply global filters to spot_df as well as df. |
lithium-supply-chain/pages/__init__.py |
Page exports. |
lithium-supply-chain/data/forecast_version/forecast_version_1/dummy_supply_data.csv |
Synthetic supply data. |
lithium-supply-chain/data/forecast_version/forecast_version_1/dummy_spot_data.csv |
Synthetic spot-market data. |
lithium-supply-chain/data/forecast_version/forecast_version_1/dummy_resource_data.csv |
Synthetic resource data. |
lithium-supply-chain/data/colors.csv |
Visualization color mappings. |
lithium-supply-chain/constants.py |
Dash app and theme configuration. |
lithium-supply-chain/assets/styles.css |
Lithium dashboard styling. |
lithium-supply-chain/assets/media_queries.css |
Responsive styling placeholder. |
lithium-supply-chain/app.py |
Lithium application entry point. |
energy-leaflet-cytoscape-visualizer/requirements.txt |
Energy app dependencies. |
energy-leaflet-cytoscape-visualizer/data.py |
Moderate: include the full documented date range. Nit: describe generated curves as half-hourly. |
energy-leaflet-cytoscape-visualizer/assets/style.css |
Energy app styling. |
energy-leaflet-cytoscape-visualizer/app.py |
Critical: fix inverse Web Mercator positioning. Moderate: register edge editing, refresh downstream data after topology edits, and reset the selected node when switching transformers. |
Review details
Suppressed comments (10)
energy-leaflet-cytoscape-visualizer/app.py:725
- Topology edits update
network-elements, but this callback reads that store asState, so removing, reversing, splitting, or linking an edge never recomputes the downstream badges/chart for the current selection. Make the network data an input so the displayed load follows the edited topology.
State("network-elements", "data"),
energy-leaflet-cytoscape-visualizer/data.py:5
- The module docstring calls these hourly load curves, but the generator creates 30-minute samples (
freq="30min"). This is misleading for consumers of the generated time series; describe them as half-hourly to match the actual data.
with realistic-looking lat/lon positions, plus synthetic hourly load
curves for every connection point (PR - Point de Raccordement).
lithium-supply-chain/pages/supply_sankey.py:183
- Only
dfis filtered here; the unchangedspot_dfis later used to add Sankey links. Consequently, selectingResource RegionorResource Companycan still render spot-market links for excluded resources, so the global filters do not apply to all rendered data. Apply compatible filters tospot_dfas well.
df = df[df[column].isin(filter)]
lithium-supply-chain/utils/chart_utils.py:65
- Correct the spelling of
seperatingin this newly added documentation.
(seperating spod nodes from lep nodes) and creates a link from this aggregation to
lithium-supply-chain/utils/chart_utils.py:110
- Correct the typo
actallyin this parameter description.
columns: list of selected "nodes" in the app, they actally represent columns in the dataframe
lithium-supply-chain/utils/chart_utils.py:113
- Remove the duplicated word
thein this parameter description.
spot_df: the the spot market dataframe
lithium-supply-chain/utils/chart_utils.py:171
- Correct the typo
diragramin this function documentation.
This function creates the data required to generate the sankey diragram.
lithium-supply-chain/utils/chart_utils.py:180
- Correct the typo
nubmersin this return-value description.
sources: a list of nubmers, each number represents a node in the sankey.
lithium-supply-chain/utils/chart_utils.py:216
- Correct the two typos in this explanatory comment:
becasueshould bebecause, andcolumsshould becolumns.
# Note: resource columns will be longer than other columns becasue spot nodes were added. By default, the loop
# will operate on the shortest column. This is what we want since the extra nodes from the resource colums come from the
lithium-supply-chain/utils/ui_utils.py:111
- Correct the missing apostrophe in this newly added comment.
# Cant delete first 2 nodes, only add "X" on third node and up.
- Files reviewed: 25/26 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @app.callback( | ||
| Output("selected-transfo", "data"), | ||
| Output("network-elements", "data", allow_duplicate=True), | ||
| Output("selected-transfo-display", "children"), | ||
| Output("current-selection-store", "data", allow_duplicate=True), | ||
| Input("transfo-search", "value"), | ||
| prevent_initial_call=True, | ||
| ) | ||
| def pick_transfo(tid): | ||
| if not tid or tid not in NETWORK: | ||
| raise PreventUpdate | ||
| return tid, NETWORK[tid], tid, {"id": tid, "kind": "node"} |
| return ids | ||
|
|
||
|
|
||
| def build_load_curves(pr_ids, start="2022-06-01", end="2023-05-31", freq="30min", seed=0): |
| assert len(targets) == nb_test_links | ||
| assert len(values) == nb_test_links | ||
| assert sorted(sources) == sorted([val[0][0] for val in get_links]) | ||
| assert fn_links.sort() == test_links.sort() |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Updated function signature to include an additional list in the return type. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Add two demo apps for lithium supply chain and energy visualizer with context menu on cytoscape on a leaflet map