Replace gravis with a modern vis.js visualization framework #177
Conversation
ParticularlyPythonicBS
added
enhancement
Feature
WalkthroughLarge refactor adding typed network-model data structures and builders, rewriting commodity-network analysis and manager to region/period semantics, introducing deterministic graph layout and vis.js export (with templates and client JS), strengthening validators and typing, adjusting hybrid loader types, updating tests, and removing two visualization dependencies and relaxing some MyPy strictness. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User / CLI
participant M as CommodityNetworkManager
participant NMD as NetworkModelData
participant CN as CommodityNetwork
participant GB as Commodity Graph Builders
participant V as Visualizer (nx_to_vis)
U->>M: analyze_network()
M->>NMD: build(...) / clone data
M->>CN: instantiate(region, period, model_data)
CN->>CN: _load_connections()
CN->>CN: analyze_network() [iterative]
Note over CN: trace backward from demands\ntrace forward from sources\nprune invalid techs until stable
CN-->>M: demand_orphans, other_orphans, valid_techs
U->>M: analyze_graphs(config)
M->>GB: visualize_graph(region, period, network_data, orphans, driven)
GB->>GB: generate_commodity_graph(...) / generate_technology_graph(...)
GB->>V: nx_to_vis(graphs, options)
V->>V: convert_graph_to_json()\ncalculate_positions()
V-->>U: export HTML (file path / optional browser)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 34
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
temoa/model_checking/validators.py (2)
112-126: Silence intentional unused parameters in validator signatures (Ruff ARG001).These functions match Pyomo callbacks; arguments are required but unused. Add noqa pragmas at defs.
-def no_slash_or_pipe(M: 'TemoaModel', element: object) -> bool: +def no_slash_or_pipe(M: 'TemoaModel', element: object) -> bool: # noqa: ARG001 @@ -def region_check(M: 'TemoaModel', region: Region) -> bool: +def region_check(M: 'TemoaModel', region: Region) -> bool: # noqa: ARG001 @@ -def emission_limit_param_check( - M: 'TemoaModel', val: float, rg: str, p: Period, e: Commodity -) -> bool: +def emission_limit_param_check( # noqa: ARG001 + M: 'TemoaModel', val: float, rg: str, p: Period, e: Commodity +) -> bool: @@ -def validate_tech_split( - M: 'TemoaModel', val: float, r: Region, p: Period, c: Commodity, t: Technology -) -> bool: +def validate_tech_split( # noqa: ARG001 + M: 'TemoaModel', val: float, r: Region, p: Period, c: Commodity, t: Technology +) -> bool: @@ -def validate_0to1(M: 'TemoaModel', val: float, *args: object) -> bool: +def validate_0to1(M: 'TemoaModel', val: float, *args: object) -> bool: # noqa: ARG001Alternatively, prefer leading underscore for unused args if it won’t break callers.
Also applies to: 128-143, 263-276, 378-396, 398-399
90-107: Avoid print in library validators; rely on logging.Printing in
validate_linked_techleaks to stdout. Replace with logging and return False.- print('Problem with Linked Tech validation: See log file') + logger.error('Problem with Linked Tech validation. See log file for details.') @@ - print('Problem with Linked Tech validation: See log file') + logger.error('Problem with Linked Tech validation. See log file for details.')Also applies to: 95-107
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
pyproject.toml(3 hunks)requirements-dev.txt(1 hunks)requirements.txt(1 hunks)temoa/data_io/hybrid_loader.py(4 hunks)temoa/model_checking/__init__.py(0 hunks)temoa/model_checking/commodity_graph.py(1 hunks)temoa/model_checking/commodity_network.py(2 hunks)temoa/model_checking/commodity_network_manager.py(1 hunks)temoa/model_checking/element_checker.py(3 hunks)temoa/model_checking/network_model_data.py(1 hunks)temoa/model_checking/pricing_check.py(2 hunks)temoa/model_checking/validators.py(17 hunks)temoa/types/core_types.py(1 hunks)temoa/utilities/graph_utils.py(1 hunks)temoa/utilities/network_vis_templates/graph_script.js(1 hunks)temoa/utilities/network_vis_templates/graph_styles.css(1 hunks)temoa/utilities/network_vis_templates/graph_template.html(1 hunks)temoa/utilities/visualizer.py(1 hunks)tests/test_network_model_data.py(2 hunks)tests/test_source_check.py(1 hunks)
💤 Files with no reviewable changes (1)
- temoa/model_checking/init.py
🧰 Additional context used
🧬 Code graph analysis (9)
tests/test_network_model_data.py (2)
temoa/model_checking/network_model_data.py (2)
_build_from_db(323-467)clone(109-111)temoa/model_checking/commodity_network.py (6)
CommodityNetwork(36-333)analyze_network(169-228)get_valid_tech(298-305)get_demand_side_orphans(307-314)get_other_orphans(316-323)unsupported_demands(325-333)
temoa/model_checking/commodity_graph.py (4)
temoa/core/config.py (1)
TemoaConfig(32-261)temoa/model_checking/network_model_data.py (2)
EfficiencyTuple(28-35)NetworkModelData(77-132)temoa/utilities/graph_utils.py (2)
calculate_initial_positions(124-179)calculate_tech_graph_positions(182-228)temoa/utilities/visualizer.py (2)
make_nx_graph(42-180)nx_to_vis(183-266)
tests/test_source_check.py (2)
temoa/model_checking/commodity_network.py (2)
CommodityNetwork(36-333)analyze_network(169-228)temoa/model_checking/network_model_data.py (1)
EfficiencyTuple(28-35)
temoa/utilities/visualizer.py (1)
temoa/utilities/graph_utils.py (1)
convert_graph_to_json(27-85)
temoa/data_io/hybrid_loader.py (2)
temoa/model_checking/element_checker.py (2)
ViableSet(18-157)filter_elements(169-251)temoa/model_checking/network_model_data.py (3)
build(137-137)build(139-139)build(140-143)
temoa/model_checking/commodity_network_manager.py (5)
temoa/core/config.py (1)
TemoaConfig(32-261)temoa/model_checking/commodity_graph.py (1)
visualize_graph(211-286)temoa/model_checking/commodity_network.py (5)
CommodityNetwork(36-333)analyze_network(169-228)unsupported_demands(325-333)get_demand_side_orphans(307-314)get_other_orphans(316-323)temoa/model_checking/element_checker.py (2)
ViableSet(18-157)exception_loc(107-108)temoa/model_checking/network_model_data.py (3)
EfficiencyTuple(28-35)NetworkModelData(77-132)clone(109-111)
temoa/model_checking/commodity_network.py (2)
temoa/model_checking/network_model_data.py (2)
EfficiencyTuple(28-35)NetworkModelData(77-132)temoa/model_checking/commodity_network_manager.py (1)
analyze_network(108-127)
temoa/model_checking/network_model_data.py (2)
temoa/core/model.py (1)
TemoaModel(97-1112)temoa/extensions/myopic/myopic_index.py (1)
MyopicIndex(37-57)
temoa/utilities/graph_utils.py (1)
temoa/model_checking/network_model_data.py (1)
EfficiencyTuple(28-35)
🪛 Ruff (0.14.1)
tests/test_network_model_data.py
170-170: Avoid specifying long messages outside the exception class
(TRY003)
250-250: Missing return type annotation for private function dispatcher
(ANN202)
256-256: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
258-258: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
268-268: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
274-274: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
276-276: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
278-278: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
280-280: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
281-281: Avoid specifying long messages outside the exception class
(TRY003)
304-304: Missing return type annotation for private function dispatcher
(ANN202)
309-309: Create your own exception
(TRY002)
309-309: Avoid specifying long messages outside the exception class
(TRY003)
321-321: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
323-323: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
329-329: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
335-335: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
337-337: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
339-339: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
341-341: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
342-342: Avoid specifying long messages outside the exception class
(TRY003)
temoa/model_checking/element_checker.py
126-126: Avoid specifying long messages outside the exception class
(TRY003)
192-192: Avoid specifying long messages outside the exception class
(TRY003)
195-197: Avoid specifying long messages outside the exception class
(TRY003)
temoa/model_checking/commodity_graph.py
139-139: Unnecessary dict comprehension for iterable; use dict.fromkeys instead
Replace with dict.fromkeys(iterable))
(C420)
261-261: Unnecessary list() call within sorted()
Remove the inner list() call
(C414)
284-284: Logging statement uses f-string
(G004)
286-286: Logging statement uses f-string
(G004)
temoa/utilities/visualizer.py
68-68: Unnecessary list() call within sorted()
Remove the inner list() call
(C414)
188-188: Boolean-typed positional argument in function definition
(FBT001)
188-188: Boolean default positional argument in function definition
(FBT002)
220-222: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
221-221: Logging statement uses f-string
(G004)
255-255: Logging statement uses f-string
(G004)
262-262: Consider moving this statement to an else block
(TRY300)
265-265: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
265-265: Logging statement uses f-string
(G004)
temoa/model_checking/validators.py
112-112: Unused function argument: M
(ARG001)
128-128: Unused function argument: M
(ARG001)
264-264: Unused function argument: val
(ARG001)
380-380: Unused function argument: val
(ARG001)
398-398: Unused function argument: M
(ARG001)
398-398: Unused function argument: args
(ARG001)
temoa/model_checking/commodity_network_manager.py
135-135: Avoid specifying long messages outside the exception class
(TRY003)
203-203: Avoid specifying long messages outside the exception class
(TRY003)
temoa/model_checking/network_model_data.py
105-107: Avoid specifying long messages outside the exception class
(TRY003)
252-252: Do not catch blind exception: Exception
(BLE001)
257-267: Possible SQL injection vector through string-based query construction
(S608)
269-277: Possible SQL injection vector through string-based query construction
(S608)
temoa/utilities/graph_utils.py
27-32: Generic function convert_graph_to_json should use type parameters
Use type parameters
(UP047)
51-51: Logging statement uses f-string
(G004)
76-76: Logging statement uses f-string
(G004)
127-127: Unused function argument: unique_sectors
(ARG001)
142-144: Unnecessary list() call within sorted()
Remove the inner list() call
(C414)
171-171: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
172-172: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
193-193: Unnecessary list() call within sorted()
Remove the inner list() call
(C414)
221-221: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
222-222: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
🔇 Additional comments (24)
requirements.txt (1)
259-259: LGTM! Dependency cleanup aligned with PR objectives.The removal of gravis and nx-vis-visualizer dependencies and the consolidation of networkx provenance comments are consistent with the PR's goal to replace the abandoned gravis library with a vis.js-based visualization.
pyproject.toml (3)
48-48: Good addition for type checking networkx usage.Adding
types-networkx>=3.5.0.20251001to dev dependencies provides proper type stubs for networkx, which is essential given networkx is now a core dependency (after removing gravis).
129-129: Temporary relaxation of MyPy strictness is acceptable.Setting
disallow_any_explicit = falseis noted as temporary to ease the transition. Consider creating a tracking issue to re-enable this setting once the typing refactor is complete.
107-107: No action required—all modules in temoa/model_checking/ have proper type annotations.Verification confirms that all public functions across the temoa/model_checking/ modules have both parameter and return type annotations (e.g.,
def __init__(self, region: Region, period: Period, model_data: NetworkModelData) -> None:,def analyze_network(self) -> bool:,def no_slash_or_pipe(M: 'TemoaModel', element: object) -> bool:). The removal of the^temoa/model_checking/exclusion from the MyPy exclude pattern is safe and will not introduce type-checking errors.requirements-dev.txt (1)
276-276: LGTM! Consistent with main requirements.The dependency cleanup in requirements-dev.txt matches the changes in requirements.txt, maintaining consistency across both files.
temoa/model_checking/element_checker.py (5)
13-15: Good typing additions for validation elements.The new type aliases
ValidationPrimitive,ValidationElement, andInputElementprovide clear, explicit typing for the validation system and improve code readability.
19-40: Excellent documentation of exception-based matching.The enhanced docstring clearly explains the two matching modes (exact and exception-based) with concrete examples, making the class's behavior immediately understandable.
62-68: Well-designed internal state management.The introduction of internal state caches (
_compiled_val_exceptions,non_excepted_items) and the_update_internalsmethod that maintains consistency across state changes is a solid design pattern that improves performance through pre-computation.Also applies to: 85-105
114-133: Good use of method chaining and immutability.The
set_val_exceptionsmethod properly validates inputs, usesfrozensetfor immutability, pre-compiles regex patterns for performance, and returnsSelffor method chaining.
210-251: Efficient two-phase filtering with exception support.The refactored filtering logic properly handles both exact matches and exception-based matches with good performance characteristics through pre-computed itemgetters and compiled regexes.
temoa/types/core_types.py (1)
16-16: LGTM! Consistent type alias addition.The
Sector = strtype alias is consistent with other dimension type aliases in this module and supports the sector-based features introduced in this PR.temoa/model_checking/pricing_check.py (2)
118-118: Good fix: Corrected typo in variable name.Renamed
compaprable_fctocomparable_fc, fixing a spelling error.
152-152: No changes needed; review comment is incorrect.The code at line 152 safely accesses
missing_techs[t]becausetcomes from the loopfor t in missing_techs:at line 144, which iterates only over keys that exist in the dictionary. When you loop directly over a dictionary withfor key in dict:, the iteration variable is guaranteed to be an existing key. Therefore, there is no risk ofKeyError, and the change from.get(t)to[t]is correct.Likely an incorrect or invalid review comment.
temoa/utilities/network_vis_templates/graph_styles.css (1)
1-29: Well-structured CSS for visualization UI.The stylesheet provides comprehensive styling for the network visualization interface with good use of modern CSS features (flexbox, transitions, responsive design). The organization is clear and maintainable.
temoa/data_io/hybrid_loader.py (4)
40-40: Good addition: Import ValidationPrimitive for proper typing.Importing
ValidationPrimitivefromelement_checkerprovides explicit typing for validation operations, improving type safety and code clarity.
341-344: Proper type safety with explicit cast.The cast to
Sequence[tuple[ValidationPrimitive, ...]]ensures type safety when passing values tofilter_elements, which expects this specific type signature.
459-469: Safer set construction using member_tuples.Switching from
.membersto.member_tuplesprovides unambiguous typing and safer set operations, especially for the construction ofviable_commsandrttsets. This aligns with the stronger typing introduced in theViableSetrefactor.
343-343: Disregard this review comment—it contains a factual error.The review incorrectly claims calls use positional arguments instead of keyword arguments. Line 343 actually uses keyword arguments (
values=,validation=,value_locations=) that match the function parameter names exactly. Line 403 does use positional arguments, but thebuildfunction signature explicitly accepts them via*args: object, **kwargs: object. Both calls are appropriate and consistent with their function signatures.Likely an incorrect or invalid review comment.
tests/test_source_check.py (2)
28-41: Good coverage across topologies; clear expectations.Test vectors exercise backward/forward traces, loops, and no-source cases via the public API. LGTM.
Also applies to: 54-69, 99-111, 118-129, 133-151, 154-167, 176-213
190-197: Minor: ensure EfficiencyTuple kwargs stay aligned with type.You’re correctly using keyword args. If you later switch to positional, match the declared order (region, input_comm, tech, vintage, output_comm, lifetime, sector). Based on learnings.
temoa/model_checking/commodity_network_manager.py (1)
198-214: Confirm intent: graphs built from original data, not filtered.You pass
network_data=self.orig_datato visualize_graph. This shows orphans on the full network (useful for triage). If the intent is to visualize the cleaned network, switch toself.filtered_data.Would you like graphs to reflect the pruned network instead?
temoa/model_checking/commodity_network.py (1)
86-91: Pass count log reports “0 pass(es)” on first success; log actual pass.- logger.debug( - 'Region %s analysis stable after %d pass(es).', - region, - pass_num - 1, - ) + logger.debug('Region %s analysis stable after %d pass(es).', region, pass_num)Likely an incorrect or invalid review comment.
temoa/model_checking/commodity_graph.py (1)
262-270: Remove redundant list() in sorted (Ruff C414).- unique_sectors = sorted(list(sector_colors.keys())) + unique_sectors = sorted(sector_colors)Likely an incorrect or invalid review comment.
temoa/model_checking/network_model_data.py (1)
16-16: This review comment is incorrect. The project requires Python 3.12+, which fully supports bothSelf(available since 3.11) and PEP 695typealiases (available since 3.12).The project explicitly requires Python 3.12 or higher as specified in pyproject.toml. Both features in question are compatible with this version requirement and are already used throughout the codebase in multiple files without issue. No changes are needed.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
tests/test_network_model_data.py (2)
251-281: Replace lambda-based MagicMocks; add return type annotation.Use
.fetchall.return_value/.fetchone.return_valueinstead offetchall=lambda: ...for clarity and Ruff compliance. Also annotatedispatcher.Apply pattern across branches (excerpt shown):
- def dispatcher(query: str, *_: object): + def dispatcher(query: str, *_: object) -> MagicMock: if 'sector FROM Technology' in query: return sector_check_mock elif 'FROM main.Efficiency' in query: return efficiency_mock elif 'Technology WHERE retire==1' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif 'FROM SurvivalCurve' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif 'FROM TimePeriod' in query: - return MagicMock(fetchall=lambda: [(2020,), (2025,)]) + m = MagicMock(); m.fetchall.return_value = [(2020,), (2025,)]; return m elif "Commodity WHERE flag LIKE '%p%'" in query: - return MagicMock(fetchall=lambda: [('s1',), ('p1',), ('d1',)]) + m = MagicMock(); m.fetchall.return_value = [('s1',), ('p1',), ('d1',)]; return m elif "Commodity WHERE flag LIKE '%w%'" in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif "Commodity WHERE flag = 's'" in query: - return MagicMock(fetchall=lambda: [('s1',)]) + m = MagicMock(); m.fetchall.return_value = [('s1',)]; return m elif 'FROM main.Demand' in query: - return MagicMock(fetchall=lambda: [('R1', 2020, 'd1')]) + m = MagicMock(); m.fetchall.return_value = [('R1', 2020, 'd1')]; return m elif 'FROM EndOfLifeOutput' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif 'FROM ConstructionInput' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif 'FROM main.LinkedTech' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif 'FROM CostVariable' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return mBased on past review comments.
301-343: Simplify missing-column simulation; remove unused mock and drop lambda-based mocks.Raise
sqlite3.OperationalErrordirectly; replace lambda MagicMocks; add return type annotation.Apply:
- def dispatcher(query: str, *_: object): + def dispatcher(query: str, *_: object) -> MagicMock: if 'sector FROM Technology' in query: - # Simulate column not existing - mock = MagicMock() - mock.execute.side_effect = Exception('no such column: sector') - raise sqlite3.OperationalError('no such column: sector') + raise sqlite3.OperationalError('no such column: sector') elif 'FROM main.Efficiency' in query: # Return data without sector column - mock = MagicMock() - mock.fetchall.return_value = [ + m = MagicMock() + m.fetchall.return_value = [ ('R1', 's1', 't1', 2000, 'p1', 100), ('R1', 'p1', 't2', 2000, 'd1', 100), ] - return mock + return m elif 'FROM main.Commodity' in query: - return MagicMock(fetchall=lambda: [('s1',), ('p1',), ('d1',)]) + m = MagicMock(); m.fetchall.return_value = [('s1',), ('p1',), ('d1',)]; return m elif 'Technology WHERE retire==1' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif 'FROM SurvivalCurve' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif 'FROM TimePeriod' in query: - return MagicMock(fetchall=lambda: [(2020,), (2025,)]) + m = MagicMock(); m.fetchall.return_value = [(2020,), (2025,)]; return m elif "Commodity WHERE flag LIKE '%p%'" in query: - return MagicMock(fetchall=lambda: [('s1',), ('p1',), ('d1',)]) + m = MagicMock(); m.fetchall.return_value = [('s1',), ('p1',), ('d1',)]; return m elif "Commodity WHERE flag LIKE '%w%'" in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif "Commodity WHERE flag = 's'" in query: - return MagicMock(fetchall=lambda: [('s1',)]) + m = MagicMock(); m.fetchall.return_value = [('s1',)]; return m elif 'FROM main.Demand' in query: - return MagicMock(fetchall=lambda: [('R1', 2020, 'd1')]) + m = MagicMock(); m.fetchall.return_value = [('R1', 2020, 'd1')]; return m elif 'FROM EndOfLifeOutput' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif 'FROM ConstructionInput' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif 'FROM main.LinkedTech' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return m elif 'FROM CostVariable' in query: - return MagicMock(fetchall=lambda: []) + m = MagicMock(); m.fetchall.return_value = []; return mBased on past review comments.
temoa/model_checking/commodity_graph.py (3)
264-264: Drop unnecessary list() inside sorted.
sorted(sector_colors.keys())is sufficient.Apply:
- unique_sectors = sorted(list(sector_colors.keys())) + unique_sectors = sorted(sector_colors.keys())
63-69: Type annotation mismatch: declares lists but uses sets.The defaultdict returns sets and
.add()is used. Update the annotation to sets for correctness and MyPy.Apply:
- commodity_map: defaultdict[str, dict[str, list[str]]] = defaultdict( - lambda: {'producers': set(), 'consumers': set()} - ) + commodity_map: defaultdict[str, dict[str, set[str]]] = defaultdict( + lambda: {'producers': set(), 'consumers': set()} + )
185-192: Use 'dashes': True for dashed edges (aligns with vis.js and serializer).Avoid custom 'style' strings; emit the vis.js-recognized flag.
Apply:
- if tech_name in driven_names: - attrs.update({'color': '#1f77b4', 'value': 3, 'style': 'dashed'}) + if tech_name in driven_names: + attrs.update({'color': '#1f77b4', 'value': 3, 'dashes': True}) driven_commodities.update({ic, oc}) - elif tech_name in other_orphan_names: - attrs.update({'color': '#ff7f0e', 'value': 4, 'style': 'dashed'}) + elif tech_name in other_orphan_names: + attrs.update({'color': '#ff7f0e', 'value': 4, 'dashes': True}) other_orphan_commodities.update({ic, oc}) - elif tech_name in demand_orphan_names: - attrs.update({'color': '#d62728', 'value': 6, 'style': 'dashed'}) + elif tech_name in demand_orphan_names: + attrs.update({'color': '#d62728', 'value': 6, 'dashes': True}) demand_orphan_commodities.update({ic, oc})
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
temoa/model_checking/commodity_graph.py(1 hunks)temoa/utilities/visualizer.py(1 hunks)tests/test_network_model_data.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
temoa/utilities/visualizer.py (1)
temoa/utilities/graph_utils.py (1)
convert_graph_to_json(27-85)
tests/test_network_model_data.py (2)
temoa/model_checking/commodity_network.py (6)
CommodityNetwork(36-333)analyze_network(169-228)get_valid_tech(298-305)get_demand_side_orphans(307-314)get_other_orphans(316-323)unsupported_demands(325-333)temoa/model_checking/network_model_data.py (2)
_build_from_db(323-467)clone(109-111)
temoa/model_checking/commodity_graph.py (4)
temoa/core/config.py (1)
TemoaConfig(32-261)temoa/model_checking/network_model_data.py (2)
EfficiencyTuple(28-35)NetworkModelData(77-132)temoa/utilities/graph_utils.py (2)
calculate_initial_positions(124-179)calculate_tech_graph_positions(182-228)temoa/utilities/visualizer.py (2)
make_nx_graph(42-180)nx_to_vis(183-267)
🪛 Ruff (0.14.1)
temoa/utilities/visualizer.py
256-256: Logging statement uses f-string
(G004)
263-263: Consider moving this statement to an else block
(TRY300)
tests/test_network_model_data.py
171-171: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Missing return type annotation for private function dispatcher
(ANN202)
257-257: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
259-259: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
265-265: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
271-271: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
273-273: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
275-275: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
277-277: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
278-278: Avoid specifying long messages outside the exception class
(TRY003)
302-302: Missing return type annotation for private function dispatcher
(ANN202)
307-307: Avoid specifying long messages outside the exception class
(TRY003)
319-319: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
321-321: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
327-327: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
333-333: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
335-335: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
337-337: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
339-339: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
340-340: Avoid specifying long messages outside the exception class
(TRY003)
temoa/model_checking/commodity_graph.py
264-264: Unnecessary list() call within sorted()
Remove the inner list() call
(C414)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: setup and test (3.12)
ParticularlyPythonicBS
force-pushed
the
feat/network_visualization_revamp
branch
from
52c06a4 to
9c0add8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
temoa/model_checking/commodity_graph.py (1)
175-193: vis.js expectsdashes, notstyle: 'dashed', so dashed edges won’t render.Switch to
dashes: trueand ensure the serializer/JS preserves it.- if tech_name in driven_names: - attrs.update({'color': '#1f77b4', 'value': 3, 'style': 'dashed'}) + if tech_name in driven_names: + attrs.update({'color': '#1f77b4', 'value': 3, 'dashes': True}) driven_commodities.update({ic, oc}) - elif tech_name in other_orphan_names: - attrs.update({'color': '#ff7f0e', 'value': 4, 'style': 'dashed'}) + elif tech_name in other_orphan_names: + attrs.update({'color': '#ff7f0e', 'value': 4, 'dashes': True}) other_orphan_commodities.update({ic, oc}) - elif tech_name in demand_orphan_names: - attrs.update({'color': '#d62728', 'value': 6, 'style': 'dashed'}) + elif tech_name in demand_orphan_names: + attrs.update({'color': '#d62728', 'value': 6, 'dashes': True}) demand_orphan_commodities.update({ic, oc})Also update the aggregator in temoa/utilities/visualizer.make_nx_graph to propagate
dasheson grouped edges (e.g., setdashes=Trueif any grouped edge has it).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
temoa/model_checking/commodity_graph.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
temoa/model_checking/commodity_graph.py (4)
temoa/core/config.py (1)
TemoaConfig(32-261)temoa/model_checking/network_model_data.py (2)
EfficiencyTuple(28-35)NetworkModelData(77-132)temoa/utilities/graph_utils.py (2)
calculate_initial_positions(124-179)calculate_tech_graph_positions(182-228)temoa/utilities/visualizer.py (2)
make_nx_graph(42-180)nx_to_vis(183-267)
🪛 Ruff (0.14.1)
temoa/model_checking/commodity_graph.py
264-264: Unnecessary list() call within sorted()
Remove the inner list() call
(C414)
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (9)
temoa/utilities/graph_utils.py (1)
230-235: Make tech-graph jitter deterministic for reproducible layouts.Currently uses random.uniform, breaking determinism promised in the module docstring.
- # Apply random "jitter" to prevent stacking - rand_angle = random.uniform(0, 2 * math.pi) - rand_radius = random.uniform(0, jitter_radius) + # Deterministic jitter per tech + seed = uuid.uuid5(uuid.NAMESPACE_DNS, str(edge_tuple.tech)).int + rng = random.Random(seed) + rand_angle = rng.uniform(0, 2 * math.pi) + rand_radius = rng.uniform(0, jitter_radius)temoa/model_checking/network_model_data.py (5)
117-124: Scope driven-techs by region to prevent cross-region bleed.Filter available_linked_techs by the requested region.
- driven_tech_names = {lt.driven for lt in self.available_linked_techs} + driven_tech_names = {lt.driven for lt in self.available_linked_techs if lt.region == region}
247-256: Avoid blindexcept Exception; catch concrete DB/mock errors.Use specific exceptions so real faults propagate.
- except sqlite3.OperationalError: - has_sector = False - except Exception: - # For mock objects in tests, assume no sector column - has_sector = False + except sqlite3.OperationalError: + has_sector = False + except (sqlite3.DatabaseError, AttributeError): + # For atypical cursors/mocks without schema + has_sector = False
257-279: Remove dynamic SQL with interpolated table; select from vetted constants.Silences S608 and avoids accidental interpolation.
- if has_sector: - query = f""" - SELECT - eff.region, eff.input_comm, eff.tech, eff.vintage, eff.output_comm, - COALESCE(lp.lifetime, lt.lifetime, ?) AS lifetime, - COALESCE(tech_dim.sector, 'Other') AS sector - FROM main.{table} AS eff - LEFT JOIN main.LifetimeProcess AS lp ON eff.tech = lp.tech AND eff.vintage = lp.vintage AND eff.region = lp.region - LEFT JOIN main.LifetimeTech AS lt ON eff.tech = lt.tech AND eff.region = lt.region - LEFT JOIN main.Technology AS tech_dim ON eff.tech = tech_dim.tech - JOIN main.TimePeriod AS tp ON eff.vintage = tp.period - """ - else: - query = f""" - SELECT - eff.region, eff.input_comm, eff.tech, eff.vintage, eff.output_comm, - COALESCE(lp.lifetime, lt.lifetime, ?) AS lifetime - FROM main.{table} AS eff - LEFT JOIN main.LifetimeProcess AS lp ON eff.tech = lp.tech AND eff.vintage = lp.vintage AND eff.region = lp.region - LEFT JOIN main.LifetimeTech AS lt ON eff.tech = lt.tech AND eff.region = lt.region - JOIN main.TimePeriod AS tp ON eff.vintage = tp.period - """ + if has_sector and table == 'Efficiency': + query = """ + SELECT + eff.region, eff.input_comm, eff.tech, eff.vintage, eff.output_comm, + COALESCE(lp.lifetime, lt.lifetime, ?) AS lifetime, + COALESCE(tech_dim.sector, 'Other') AS sector + FROM main.Efficiency AS eff + LEFT JOIN main.LifetimeProcess AS lp ON eff.tech = lp.tech AND eff.vintage = lp.vintage AND eff.region = lp.region + LEFT JOIN main.LifetimeTech AS lt ON eff.tech = lt.tech AND eff.region = lt.region + LEFT JOIN main.Technology AS tech_dim ON eff.tech = tech_dim.tech + JOIN main.TimePeriod AS tp ON eff.vintage = tp.period + """ + elif has_sector and table == 'MyopicEfficiency': + query = """ + SELECT + eff.region, eff.input_comm, eff.tech, eff.vintage, eff.output_comm, + COALESCE(lp.lifetime, lt.lifetime, ?) AS lifetime, + COALESCE(tech_dim.sector, 'Other') AS sector + FROM main.MyopicEfficiency AS eff + LEFT JOIN main.LifetimeProcess AS lp ON eff.tech = lp.tech AND eff.vintage = lp.vintage AND eff.region = lp.region + LEFT JOIN main.LifetimeTech AS lt ON eff.tech = lt.tech AND eff.region = lt.region + LEFT JOIN main.Technology AS tech_dim ON eff.tech = tech_dim.tech + JOIN main.TimePeriod AS tp ON eff.vintage = tp.period + """ + elif not has_sector and table == 'Efficiency': + query = """ + SELECT + eff.region, eff.input_comm, eff.tech, eff.vintage, eff.output_comm, + COALESCE(lp.lifetime, lt.lifetime, ?) AS lifetime + FROM main.Efficiency AS eff + LEFT JOIN main.LifetimeProcess AS lp ON eff.tech = lp.tech AND eff.vintage = lp.vintage AND eff.region = lp.region + LEFT JOIN main.LifetimeTech AS lt ON eff.tech = lt.tech AND eff.region = lt.region + JOIN main.TimePeriod AS tp ON eff.vintage = tp.period + """ + else: # not has_sector and table == 'MyopicEfficiency' + query = """ + SELECT + eff.region, eff.input_comm, eff.tech, eff.vintage, eff.output_comm, + COALESCE(lp.lifetime, lt.lifetime, ?) AS lifetime + FROM main.MyopicEfficiency AS eff + LEFT JOIN main.LifetimeProcess AS lp ON eff.tech = lp.tech AND eff.vintage = lp.vintage AND eff.region = lp.region + LEFT JOIN main.LifetimeTech AS lt ON eff.tech = lt.tech AND eff.region = lt.region + JOIN main.TimePeriod AS tp ON eff.vintage = tp.period + """
358-361: Guard inter-regional parsing; split once.Avoid mis-splits if region has multiple hyphens.
- if '-' in r: # Inter-regional transfer - r1, r2 = r.split('-') + if '-' in r and r.count('-') == 1: # Inter-regional transfer + r1, r2 = r.split('-', 1)
442-452: Critical: lifetime is undefined in Construction block.
lifetimeisn’t set in this scope; pick a deterministic value.- for r, ic, tech, v in lookup_data['construction']: - res.available_techs[r, v].add( + for r, ic, tech, v in lookup_data['construction']: + construction_lifetime = basic_data['period_length'].get(v, 1) + res.available_techs[r, v].add( EdgeTuple( region=r, input_comm=ic, tech='Construction', vintage=v, output_comm=tech, - lifetime=lifetime, + lifetime=construction_lifetime, sector='Other', ) )temoa/model_checking/commodity_network_manager.py (1)
85-91: Fix off‑by‑one in stability log.You log pass_num - 1; first successful pass reports 0. Use pass_num.
- logger.debug( - 'Region %s analysis stable after %d pass(es).', - region, - pass_num - 1, - ) + logger.debug( + 'Region %s analysis stable after %d pass(es).', + region, + pass_num, + )temoa/model_checking/commodity_network.py (1)
57-64: Guard must be region/period‑specific, not global.Current check may pass/fail incorrectly. Validate for (self.region, self.period).
- if not self.model_data.source_commodities: + if not self.model_data.source_commodities[self.region, self.period]: msg = ( "No source commodities found. Have they been marked with 's' " 'in the commodities table?' ) logger.error(msg) raise ValueError(msg)temoa/utilities/visualizer.py (1)
255-263: Avoid f-strings in logs; use parameterized logging.Conforms to Ruff G004 and previous guidance.
- logger.info(f'Generated graph HTML at: {abs_path}') + logger.info('Generated graph HTML at: %s', abs_path)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
temoa/model_checking/commodity_graph.py(1 hunks)temoa/model_checking/commodity_network.py(2 hunks)temoa/model_checking/commodity_network_manager.py(1 hunks)temoa/model_checking/network_model_data.py(1 hunks)temoa/utilities/graph_utils.py(1 hunks)temoa/utilities/visualizer.py(1 hunks)tests/test_network_model_data.py(2 hunks)tests/test_source_check.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
tests/test_source_check.py (2)
temoa/model_checking/commodity_network.py (2)
CommodityNetwork(36-333)analyze_network(169-228)temoa/model_checking/network_model_data.py (1)
EdgeTuple(28-35)
tests/test_network_model_data.py (2)
temoa/model_checking/commodity_network.py (6)
CommodityNetwork(36-333)analyze_network(169-228)get_valid_tech(298-305)get_demand_side_orphans(307-314)get_other_orphans(316-323)unsupported_demands(325-333)temoa/model_checking/network_model_data.py (1)
_build_from_db(324-468)
temoa/model_checking/commodity_graph.py (4)
temoa/core/config.py (1)
TemoaConfig(32-261)temoa/model_checking/network_model_data.py (2)
EdgeTuple(28-35)NetworkModelData(77-133)temoa/utilities/graph_utils.py (2)
calculate_initial_positions(127-189)calculate_tech_graph_positions(192-238)temoa/utilities/visualizer.py (2)
make_nx_graph(42-180)nx_to_vis(183-267)
temoa/utilities/graph_utils.py (2)
temoa/model_checking/network_model_data.py (1)
EdgeTuple(28-35)temoa/utilities/network_vis_templates/graph_script.js (3)
nodes(46-46)data(3-3)edges(47-47)
temoa/model_checking/commodity_network.py (1)
temoa/model_checking/network_model_data.py (2)
EdgeTuple(28-35)NetworkModelData(77-133)
temoa/utilities/visualizer.py (1)
temoa/utilities/graph_utils.py (1)
convert_graph_to_json(27-88)
temoa/model_checking/network_model_data.py (2)
temoa/core/model.py (1)
TemoaModel(97-1112)temoa/extensions/myopic/myopic_index.py (1)
MyopicIndex(37-57)
temoa/model_checking/commodity_network_manager.py (5)
temoa/core/config.py (1)
TemoaConfig(32-261)temoa/model_checking/commodity_graph.py (1)
visualize_graph(215-295)temoa/model_checking/commodity_network.py (5)
CommodityNetwork(36-333)analyze_network(169-228)unsupported_demands(325-333)get_demand_side_orphans(307-314)get_other_orphans(316-323)temoa/model_checking/element_checker.py (2)
ViableSet(18-157)exception_loc(107-108)temoa/model_checking/network_model_data.py (3)
EdgeTuple(28-35)NetworkModelData(77-133)clone(109-111)
🪛 Ruff (0.14.1)
tests/test_network_model_data.py
251-251: Missing return type annotation for private function dispatcher
(ANN202)
257-257: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
259-259: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
265-265: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
271-271: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
273-273: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
275-275: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
277-277: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
278-278: Avoid specifying long messages outside the exception class
(TRY003)
302-302: Missing return type annotation for private function dispatcher
(ANN202)
307-307: Avoid specifying long messages outside the exception class
(TRY003)
319-319: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
321-321: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
327-327: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
333-333: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
335-335: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
337-337: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
339-339: Prefer list over useless lambda
Replace with lambda with list
(PIE807)
340-340: Avoid specifying long messages outside the exception class
(TRY003)
temoa/utilities/graph_utils.py
27-32: Generic function convert_graph_to_json should use type parameters
Use type parameters
(UP047)
51-51: Logging statement uses f-string
(G004)
76-76: Logging statement uses f-string
(G004)
203-203: Unnecessary list() call within sorted()
Remove the inner list() call
(C414)
231-231: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
232-232: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
temoa/utilities/visualizer.py
256-256: Logging statement uses f-string
(G004)
263-263: Consider moving this statement to an else block
(TRY300)
temoa/model_checking/network_model_data.py
105-107: Avoid specifying long messages outside the exception class
(TRY003)
253-253: Do not catch blind exception: Exception
(BLE001)
258-268: Possible SQL injection vector through string-based query construction
(S608)
270-278: Possible SQL injection vector through string-based query construction
(S608)
temoa/model_checking/commodity_network_manager.py
135-135: Avoid specifying long messages outside the exception class
(TRY003)
203-203: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: setup and test (3.12)
🔇 Additional comments (3)
tests/test_source_check.py (1)
28-151: Good scenario coverage and clear expectations.Parametrization is clean; mocks align with API. No issues from my side.
temoa/model_checking/commodity_network_manager.py (2)
206-214: Confirm intent: visualize graphs from original vs. filtered data.analyze_graphs passes network_data=self.orig_data while overlaying orphans. If you intended to show only valid techs, switch to self.filtered_data. Otherwise, keep as-is.
26-31: Replace PEP 695typealias withTypeAliasfor Python <3.12 compatibility.Using
type RegionPeriodKey = ...is 3.12+. Safer to use typing.TypeAlias.-from typing import Any +from typing import Any, TypeAlias @@ -# Type alias for clarity in dictionary keys -type RegionPeriodKey = tuple[Region, Period] +# Type alias for clarity in dictionary keys +RegionPeriodKey: TypeAlias = tuple[Region, Period]If the project mandates Python 3.12+, ignore this change; otherwise adopt for broader support.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
temoa/utilities/graph_utils.py (3)
51-53: Use parameterized logging; drop f-strings.Switch to logger placeholders to avoid formatting when disabled and satisfy linters.
- logger.debug( - f"Node {node_id_str} attr '{key}' not JSON serializable, converting to string." - ) + logger.debug( + "Node %s attr '%s' not JSON serializable, converting to string.", + node_id_str, + key, + )
75-77: Same: parameterized logging for edges.Avoid f-strings in logs.
- logger.debug( - f"Edge ({u_obj}-{v_obj}) attr '{key}' not JSON serializable, converting to string." - ) + logger.debug( + "Edge (%s-%s) attr '%s' not JSON serializable, converting to string.", + u_obj, + v_obj, + key, + )
228-231: Make tech-graph jitter deterministic to honor module docs.Use a stable per-tech seed so initial layouts are reproducible between runs.
- # Apply random "jitter" to prevent stacking - rand_angle = random.uniform(0, 2 * math.pi) - rand_radius = random.uniform(0, jitter_radius) + # Apply deterministic "jitter" to prevent stacking (stable per-tech) + seed = uuid.uuid5(uuid.NAMESPACE_DNS, str(edge_tuple.tech)).int + rng = random.Random(seed) + rand_angle = rng.uniform(0, 2 * math.pi) + rand_radius = rng.uniform(0, jitter_radius)temoa/model_checking/commodity_network_manager.py (2)
119-124: Deterministic region iteration; remove redundant sorted() inside set.Store as a set; iterate regions in sorted order for stable outputs.
- self.regions = set(sorted({r for (r, p) in self.orig_data.available_techs if '-' not in r})) + self.regions = {r for (r, _p) in self.orig_data.available_techs if '-' not in r} @@ - for region in self.regions: + for region in sorted(self.regions): logger.info('Starting network analysis for region %s', region) self._analyze_region(region, data=self.filtered_data)
86-91: Fix pass count in stability log.Report the actual iteration number, not pass_num - 1.
- logger.debug( - 'Region %s analysis stable after %d pass(es).', - region, - pass_num - 1, - ) + logger.debug( + 'Region %s analysis stable after %d pass(es).', + region, + pass_num, + )temoa/model_checking/commodity_graph.py (1)
79-101: Deduplicate roles and aggregate edges to prevent O(N²) explosion in Technology View.Use sets for producers/consumers and emit one edge per (producer, consumer) with combined labels.
- commodity_map: defaultdict[str, dict[str, list[str]]] = defaultdict( - lambda: {'producers': [], 'consumers': []} - ) + commodity_map: defaultdict[str, dict[str, set[str]]] = defaultdict( + lambda: {'producers': set(), 'consumers': set()} + ) @@ - commodity_map[tech_tuple.output_comm]['producers'].append(tech_tuple.tech) - commodity_map[tech_tuple.input_comm]['consumers'].append(tech_tuple.tech) + commodity_map[tech_tuple.output_comm]['producers'].add(tech_tuple.tech) + commodity_map[tech_tuple.input_comm]['consumers'].add(tech_tuple.tech) @@ - for commodity, roles in commodity_map.items(): - if commodity in source_commodities or commodity in demand_commodities: - continue - for producer in roles['producers']: - for consumer in roles['consumers']: - if producer != consumer: - tg.add_edge( - producer, - consumer, - label=commodity, - title=f'Commodity Flow: {commodity}', - arrows='to', - color='#555555', - ) + aggregated: dict[tuple[str, str], set[str]] = {} + for commodity, roles in commodity_map.items(): + if commodity in source_commodities or commodity in demand_commodities: + continue + for producer in roles['producers']: + for consumer in roles['consumers']: + if producer == consumer: + continue + aggregated.setdefault((producer, consumer), set()).add(commodity) + for (producer, consumer), commodities in aggregated.items(): + label = ', '.join(sorted(commodities)) + tg.add_edge( + producer, + consumer, + label=label, + title=f'Commodity Flows: {label}', + arrows='to', + color='#555555', + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
temoa/model_checking/commodity_graph.py(1 hunks)temoa/model_checking/commodity_network_manager.py(1 hunks)temoa/utilities/graph_utils.py(1 hunks)temoa/utilities/network_vis_templates/graph_styles.css(1 hunks)temoa/utilities/visualizer.py(1 hunks)tests/test_network_model_data.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
temoa/model_checking/commodity_graph.py (4)
temoa/core/config.py (1)
TemoaConfig(32-261)temoa/model_checking/network_model_data.py (2)
EdgeTuple(28-35)NetworkModelData(77-133)temoa/utilities/graph_utils.py (2)
calculate_initial_positions(125-187)calculate_tech_graph_positions(190-236)temoa/utilities/visualizer.py (2)
make_nx_graph(42-179)nx_to_vis(182-258)
temoa/model_checking/commodity_network_manager.py (3)
temoa/model_checking/commodity_graph.py (1)
visualize_graph(230-310)temoa/model_checking/commodity_network.py (2)
get_demand_side_orphans(307-314)get_other_orphans(316-323)temoa/model_checking/network_model_data.py (3)
EdgeTuple(28-35)NetworkModelData(77-133)clone(109-111)
temoa/utilities/visualizer.py (1)
temoa/utilities/graph_utils.py (1)
convert_graph_to_json(27-86)
temoa/utilities/graph_utils.py (2)
temoa/model_checking/network_model_data.py (1)
EdgeTuple(28-35)temoa/utilities/network_vis_templates/graph_script.js (3)
nodes(46-46)data(3-3)edges(47-47)
tests/test_network_model_data.py (2)
temoa/model_checking/commodity_network.py (6)
CommodityNetwork(36-333)analyze_network(169-228)get_valid_tech(298-305)get_demand_side_orphans(307-314)get_other_orphans(316-323)unsupported_demands(325-333)temoa/model_checking/network_model_data.py (2)
_build_from_db(324-468)clone(109-111)
🪛 Ruff (0.14.1)
temoa/model_checking/commodity_network_manager.py
119-119: Unnecessary sorted() call within set()
Remove the inner sorted() call
(C414)
135-135: Avoid specifying long messages outside the exception class
(TRY003)
203-203: Avoid specifying long messages outside the exception class
(TRY003)
temoa/utilities/graph_utils.py
27-32: Generic function convert_graph_to_json should use type parameters
Use type parameters
(UP047)
52-52: Logging statement uses f-string
(G004)
76-76: Logging statement uses f-string
(G004)
201-201: Unnecessary list() call within sorted()
Remove the inner list() call
(C414)
229-229: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
230-230: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
tests/test_network_model_data.py
300-300: Avoid specifying long messages outside the exception class
(TRY003)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
386-386: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: setup and test (3.12)
🔇 Additional comments (1)
temoa/utilities/network_vis_templates/graph_styles.css (1)
1-29: LGTM — styles are coherent and scoped.No blocking issues found.
ParticularlyPythonicBS
force-pushed
the
feat/network_visualization_revamp
branch
from
facd2c7 to
d9c73b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
temoa/utilities/network_vis_templates/graph_template.html (2)
18-20: Add visible text for buttons (keep ARIA).Empty buttons harm discoverability; include text or visually-hidden labels.
- <button class="config-toggle-btn" aria-label="Toggle configuration panel" aria-expanded="true" - aria-controls="config-container-content"></button> + <button class="config-toggle-btn" aria-label="Toggle configuration panel" aria-expanded="true" + aria-controls="config-container-content">Toggle configuration</button> @@ - <button id="view-toggle-btn" aria-pressed="false"></button> + <button id="view-toggle-btn" aria-pressed="false">Switch view</button>Also applies to: 35-36
8-10: Critical: Invalid SRI hash blocks vis-network load.The integrity value is malformed (“%” suffix). Replace with a real sha384 hash in CI or remove until set.
- <script src="https://unpkg.com/vis-network@9.1.6/standalone/umd/vis-network.min.js" - integrity="Ux6phic9PEHJ38YtrijhkzyJ8yQlH8i/+buBR8s3mAZOJrP1gwyvAcIYl3GWtpX1%" crossorigin="anonymous" - defer></script> + <!-- Pin exact version and supply a real SRI at build time --> + <script + src="https://unpkg.com/vis-network@9.1.6/standalone/umd/vis-network.min.js" + integrity="sha384-__REPLACE_WITH_REAL_HASH_IN_CI__" + crossorigin="anonymous" + defer + ></script>temoa/model_checking/network_model_data.py (2)
117-124: Scope driven-tech selection by region to avoid cross-region bleed.- driven_tech_names = {lt.driven for lt in self.available_linked_techs} + driven_tech_names = {lt.driven for lt in self.available_linked_techs if lt.region == region}
359-363: Defensive split for inter-regional parsing.Use a single split to match the count==1 guard.
- r1, r2 = r.split('-') + r1, r2 = r.split('-', 1)temoa/utilities/network_vis_templates/graph_script.js (2)
19-31: Harden options parsing; handle string vs object.If options_json_str arrives as a JSON string, parsing failure will break configure usage.
- const { + const { nodes_json_primary: allNodesPrimary, edges_json_primary: allEdgesPrimary, nodes_json_secondary: allNodesSecondary, edges_json_secondary: allEdgesSecondary, - options_json_str: optionsObject, + options_json_str: optionsRaw, sectors_json_str: allSectors, color_legend_json_str: colorLegendData, style_legend_json_str: styleLegendData, primary_view_name: primaryViewName, secondary_view_name: secondaryViewName, } = data; + const optionsObject = (typeof optionsRaw === 'string') ? JSON.parse(optionsRaw) : optionsRaw;
254-254: Remove redundant label assignment.This overrides earlier logic and can show an incorrect target view label after init.
- viewToggleButton.textContent = `Switch to ${secondaryViewName}`;tests/test_network_model_data.py (1)
386-387: Raise AssertionError for unexpected SQL in test dispatcher.Signals test setup failure clearly.
- raise ValueError(f'Mock database received unexpected query: {query}') + raise AssertionError('Mock database received unexpected query: ' + query)temoa/model_checking/commodity_graph.py (2)
87-100: Consider aggregating commodity flows per technology pair to reduce edge count.The current implementation creates one edge per commodity flowing between each producer-consumer pair. For models with many commodities between the same technology pair, this can create visual clutter and performance overhead.
Optional optimization: Aggregate commodities and create a single edge per (producer, consumer) pair with a combined label:
+ # Aggregate commodities per producer-consumer pair + aggregated: dict[tuple[str, str], set[str]] = {} for commodity, roles in commodity_map.items(): if commodity in source_commodities or commodity in demand_commodities: continue for producer in roles['producers']: for consumer in roles['consumers']: - if producer != consumer: - tg.add_edge( - producer, - consumer, - label=commodity, - title=f'Commodity Flow: {commodity}', - arrows='to', - color='#555555', - ) + if producer == consumer: + continue + aggregated.setdefault((producer, consumer), set()).add(commodity) + + for (producer, consumer), comms in aggregated.items(): + label = ', '.join(sorted(comms)) + tg.add_edge( + producer, + consumer, + label=label, + title=f'Commodity Flows: {label}', + arrows='to', + color='#555555', + )
80-100: Use sets to deduplicate producer/consumer tech names and prevent duplicate edges.Lines 80-85 use
list[str]with.append(), allowing duplicate technology names (e.g., same tech across different vintages). The nested loops at lines 90-100 then create one edge per producer-consumer pair for each occurrence, leading to duplicate parallel edges and O(P×C) explosion.Apply this diff to deduplicate:
- commodity_map: defaultdict[str, dict[str, list[str]]] = defaultdict( - lambda: {'producers': [], 'consumers': []} + commodity_map: defaultdict[str, dict[str, set[str]]] = defaultdict( + lambda: {'producers': set(), 'consumers': set()} ) for tech_tuple in all_edges: - commodity_map[tech_tuple.output_comm]['producers'].append(tech_tuple.tech) - commodity_map[tech_tuple.input_comm]['consumers'].append(tech_tuple.tech) + commodity_map[tech_tuple.output_comm]['producers'].add(tech_tuple.tech) + commodity_map[tech_tuple.input_comm]['consumers'].add(tech_tuple.tech)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
temoa/model_checking/commodity_graph.py(1 hunks)temoa/model_checking/network_model_data.py(1 hunks)temoa/utilities/graph_utils.py(1 hunks)temoa/utilities/network_vis_templates/graph_script.js(1 hunks)temoa/utilities/network_vis_templates/graph_template.html(1 hunks)temoa/utilities/visualizer.py(1 hunks)tests/test_network_model_data.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
temoa/utilities/graph_utils.py (1)
temoa/model_checking/network_model_data.py (1)
EdgeTuple(28-35)
temoa/utilities/visualizer.py (1)
temoa/utilities/graph_utils.py (1)
convert_graph_to_json(27-91)
tests/test_network_model_data.py (3)
temoa/model_checking/commodity_network.py (6)
CommodityNetwork(36-333)analyze_network(169-228)get_valid_tech(298-305)get_demand_side_orphans(307-314)get_other_orphans(316-323)unsupported_demands(325-333)temoa/model_checking/network_model_data.py (2)
_build_from_db(324-469)clone(109-111)temoa/model_checking/commodity_network_manager.py (1)
analyze_network(108-127)
temoa/model_checking/commodity_graph.py (4)
temoa/core/config.py (1)
TemoaConfig(32-261)temoa/model_checking/network_model_data.py (2)
EdgeTuple(28-35)NetworkModelData(77-133)temoa/utilities/graph_utils.py (2)
calculate_initial_positions(130-192)calculate_tech_graph_positions(195-243)temoa/utilities/visualizer.py (2)
make_nx_graph(43-181)nx_to_vis(184-260)
temoa/model_checking/network_model_data.py (2)
temoa/core/model.py (1)
TemoaModel(97-1112)temoa/extensions/myopic/myopic_index.py (1)
MyopicIndex(37-57)
🪛 Ruff (0.14.1)
temoa/utilities/graph_utils.py
27-32: Generic function convert_graph_to_json should use type parameters
Use type parameters
(UP047)
235-235: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
tests/test_network_model_data.py
166-166: Avoid specifying long messages outside the exception class
(TRY003)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
386-386: Avoid specifying long messages outside the exception class
(TRY003)
temoa/model_checking/network_model_data.py
105-107: Avoid specifying long messages outside the exception class
(TRY003)
258-268: Possible SQL injection vector through string-based query construction
(S608)
270-278: Possible SQL injection vector through string-based query construction
(S608)
🔇 Additional comments (8)
temoa/model_checking/commodity_graph.py (3)
22-77: LGTM: Two-pass approach correctly creates one node per technology.The aggregation logic properly collects is_source/is_demand/sector flags across all edge tuples, and the node creation uses conditional styling. The nested
colorobject structure is vis.js-compatible.Minor observation: Line 69-76 constructs
node_attrsbut doesn't show a conditional'group'key addition. Sincegroup_nameisn't defined in this snippet and the relevant_code_snippets don't show this pattern, verify thatgroupis only added when sector is not None, consistent with the pattern at line 71 where sector is conditionally appended to title.
104-227: LGTM: Commodity graph generation is well-structured.The function properly uses
RegionandPeriodtypes, efficiently constructs the layer map withdict.fromkeys(line 157-159), and correctly sets'dashes': Truefor vis.js compatibility (lines 201, 204, 207). The sector-based color mapping and position calculation are cleanly integrated.
230-310: LGTM: Visualization orchestration is clean and follows logging best practices.The function properly gates on
config.plot_commodity_network(line 244), uses parameterized logging throughout (lines 245, 298, 300, 308, 310), and includes exception context withexc_info=True(line 310). The dual-graph generation and style legend construction are well-organized.temoa/utilities/visualizer.py (5)
32-40: LGTM: Clean recursive deep merge implementation.The function correctly handles nested dictionaries and returns the mutated destination as documented.
43-126: LGTM: Node creation uses vis.js-compatible nested color structure.Line 61 correctly materializes the iterable to prevent exhaustion across multiple passes. Lines 88-111 properly use nested
colorobjects withborderandbackgroundkeys, which is the correct vis.js format. Shadow configuration (lines 115) also uses the proper nested structure.
127-181: LGTM: Edge grouping logic correctly handles sector coloring and dashing.The grouped edge creation properly combines labels (lines 139-143), aggregates tooltips (lines 145-162), and correctly selects color from a technology that has the sector (line 170). Line 174 correctly uses
'dashes': Truefor vis.js compatibility.
184-260: LGTM: Function properly handles options merging, templates, and file I/O.Line 197 correctly makes
show_browserkeyword-only. Line 212 usescopy.deepcopyto prevent mutation ofDEFAULT_VIS_OPTIONS. Exception handlers (lines 221-225, 251-253) correctly returnNoneon failure. The try/except/else structure (lines 241-260) properly separates I/O operations from the success path, and logging uses parameterized format (line 255).
263-305: LGTM: Comprehensive default vis.js options with sensible defaults.The configuration properly structures nested options for nodes, edges, physics (disabled by default at line 277), interaction, and layout. The configure panel setup (lines 300-304) appropriately hides the default button while enabling the panel.
ParticularlyPythonicBS
force-pushed
the
feat/network_visualization_revamp
branch
from
10123dc to
f86ebe7
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
temoa/model_checking/pricing_check.py (1)
246-249: Critical: warnings aggregation inverts the result; issues can be masked.
warnings &= check_tech_uncap(M) will keep warnings False even if check_tech_uncap returns False when no prior warnings. Use OR with negation.- warnings &= check_tech_uncap(M) + warnings |= not check_tech_uncap(M)temoa/model_checking/validators.py (3)
112-126: Silence unused-arg warnings and validate all values.
- Several validators keep Pyomo’s (M, val, …) signature but don’t use some args; silence ARG001 and ensure val is checked consistently.
-def no_slash_or_pipe(M: 'TemoaModel', element: object) -> bool: +def no_slash_or_pipe(_: 'TemoaModel', element: object) -> bool: # noqa: ARG001 @@ -def region_check(M: 'TemoaModel', region: Region) -> bool: +def region_check(_: 'TemoaModel', region: Region) -> bool: # noqa: ARG001 @@ -def emission_limit_param_check( - M: 'TemoaModel', val: float, rg: str, p: Period, e: Commodity -) -> bool: +def emission_limit_param_check( + M: 'TemoaModel', val: float, rg: str, p: Period, e: Commodity +) -> bool: @@ - return all((region_group_check(M, rg), p in M.time_optimize, e in M.commodity_emissions)) + return all(( + val in NonNegativeReals, + region_group_check(M, rg), + p in M.time_optimize, + e in M.commodity_emissions, + )) @@ -def validate_tech_split( - M: 'TemoaModel', val: float, r: Region, p: Period, c: Commodity, t: Technology -) -> bool: +def validate_tech_split( + M: 'TemoaModel', val: float, r: Region, p: Period, c: Commodity, t: Technology +) -> bool: @@ - if all( + if all( ( + 0.0 <= val, # non-negative split r in M.regions, p in M.time_optimize, c in M.commodity_physical, t in M.tech_all, ) ): return True @@ -def validate_0to1(M: 'TemoaModel', val: float, *args: object) -> bool: - return 0.0 <= val <= 1.0 +def validate_0to1(_: 'TemoaModel', val: float, *_: object) -> bool: # noqa: ARG001 + return 0.0 <= val <= 1.0Also applies to: 128-143, 263-276, 379-396, 398-399
315-339: Accept ints and other real numbers for efficiency; drop prints.
- isinstance(val, float) rejects ints; use numbers.Real.
- Prefer logging over print().
- if all( - ( - isinstance(val, float), + if all( + ( + isinstance(val, Real), val > 0, @@ - print('Element Validations:') + logger.debug('Element Validations:') @@ - print('region', r in M.regionalIndices) - print('input_commodity', si in M.commodity_physical) - print('tech', t in M.tech_all) - print('vintage', v in M.vintage_all) - print('output_commodity', so in M.commodity_carrier) + logger.debug('region %s', r in M.regionalIndices) + logger.debug('input_commodity %s', si in M.commodity_physical) + logger.debug('tech %s', t in M.tech_all) + logger.debug('vintage %s', v in M.vintage_all) + logger.debug('output_commodity %s', so in M.commodity_carrier)Additional import needed:
from numbers import Real
95-107: Remove user-facing prints in validators.
These prints duplicate logger.error; keep logging only.- print('Problem with Linked Tech validation: See log file') + # see error log for details @@ - print('Problem with Linked Tech validation: See log file') + # see error log for details
♻️ Duplicate comments (11)
temoa/utilities/network_vis_templates/graph_script.js (2)
208-223: Don’t erase layout memory on reset; only clear filters and refit.Nulling primary/secondary positions defeats per‑view “layout memory.” Keep stored positions.
function resetView() { searchInput.value = ""; - primaryViewPositions = null; - secondaryViewPositions = null; if (currentView !== 'primary') { switchView(); // This will switch back to primary and apply null positions } else { // If already on primary, just reload the original data nodes.clear(); edges.clear(); nodes.add(allNodesPrimary); edges.add(allEdgesPrimary); - applyPositions(primaryViewPositions); // Apply null to reset + applyPositions(primaryViewPositions); network.fit(); } sectorTogglesContainer.querySelectorAll('input[type=checkbox]').forEach(c => c.checked = true); applyAllFilters(); }
64-69: Persist positions as the user drags.Update per‑view position memory on dragEnd to honor “layout memory.”
const network = new vis.Network(graphContainer, { nodes, edges }, optionsObject); +network.on('dragEnd', () => { + if (currentView === 'primary') { + primaryViewPositions = network.getPositions(); + } else { + secondaryViewPositions = network.getPositions(); + } +});temoa/utilities/network_vis_templates/graph_template.html (2)
8-8: Pin with SRI and defer for supply‑chain safety and faster paint.Add integrity/crossorigin and defer on the CDN script. Replace the placeholder hash in CI.
- <script src="https://unpkg.com/vis-network@9.1.6/standalone/umd/vis-network.min.js"></script> + <!-- Pin version, add SRI, and defer. Replace INTEGRITY_HASH via CI. --> + <script + src="https://unpkg.com/vis-network@9.1.6/standalone/umd/vis-network.min.js" + integrity="INTEGRITY_HASH" + crossorigin="anonymous" + defer + ></script>
33-34: Provide initial visible text for the view toggle button.Avoid an empty control before JS runs; enhances accessibility and progressive rendering.
- <button id="view-toggle-btn" aria-pressed="false"></button> + <button id="view-toggle-btn" aria-pressed="false">Switch view</button>tests/test_network_model_data.py (1)
386-386: Use AssertionError for unexpected SQL (consistent test failure).Unify with other dispatchers and make failures clearly assertion errors.
- raise ValueError(f'Mock database received unexpected query: {query}') + raise AssertionError('Unexpected SQL: ' + query)temoa/model_checking/commodity_graph.py (1)
87-101: Aggregate commodities per (producer, consumer) to avoid edge explosion.Emit one edge per pair with a combined label/title; dramatically reduces edges and DOM load.
- for commodity, roles in commodity_map.items(): - if commodity in source_commodities or commodity in demand_commodities: - continue - for producer in roles['producers']: - for consumer in roles['consumers']: - if producer != consumer: - tg.add_edge( - producer, - consumer, - label=commodity, - title=f'Commodity Flow: {commodity}', - arrows='to', - color='#555555', - ) + aggregated: dict[tuple[str, str], set[str]] = {} + for commodity, roles in commodity_map.items(): + if commodity in source_commodities or commodity in demand_commodities: + continue + for producer in roles['producers']: + for consumer in roles['consumers']: + if producer == consumer: + continue + aggregated.setdefault((producer, consumer), set()).add(commodity) + for (producer, consumer), comms in aggregated.items(): + label = ', '.join(sorted(comms)) + tg.add_edge( + producer, + consumer, + label=label, + title=f'Commodity Flows: {label}', + arrows='to', + color='#555555', + )temoa/model_checking/commodity_network_manager.py (2)
85-91: Fix pass-count logging (off by one).It logs
pass_num - 1; should log the actual iteration count.- logger.debug( - 'Region %s analysis stable after %d pass(es).', - region, - pass_num - 1, - ) + logger.debug( + 'Region %s analysis stable after %d pass%s.', + region, + pass_num, + '' if pass_num == 1 else 'es', + )
198-206: Deterministic graph generation order.Iterate regions in
sorted(self.regions)for stable builds/logs.- for region in self.regions: + for region in sorted(self.regions): for period in self.periods: visualize_graph(temoa/model_checking/commodity_network.py (2)
57-64: Bug: source check must be region/period-specific.Global check can raise on valid cases or miss empty per-key sets. Guard on
(self.region, self.period).- if not self.model_data.source_commodities: + if not self.model_data.source_commodities[self.region, self.period]: msg = ( - "No source commodities found. Have they been marked with 's' " - 'in the commodities table?' + "No source commodities found for region %s, period %s. " + "Have they been marked with 's' in the commodities table?" ) - logger.error(msg) - raise ValueError(msg) + logger.error(msg, self.region, self.period) + raise ValueError(msg % (self.region, self.period))
26-33: Clarify link directionality (optional).
TechLinkserves both input→tech and tech→output contexts; considerInputLink/OutputLinkaliases or comments to disambiguate.temoa/model_checking/network_model_data.py (1)
117-125: Scope driven-tech lookup by region.Avoid cross‑region bleed by filtering
available_linked_techsto the current region.- driven_tech_names = {lt.driven for lt in self.available_linked_techs} + driven_tech_names = { + lt.driven for lt in self.available_linked_techs if lt.region == region + } return { efficiencyTuple for efficiencyTuple in self.available_techs.get((region, period), set()) if efficiencyTuple.tech in driven_tech_names }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
pyproject.toml(3 hunks)requirements-dev.txt(1 hunks)requirements.txt(1 hunks)temoa/data_io/hybrid_loader.py(4 hunks)temoa/model_checking/__init__.py(0 hunks)temoa/model_checking/commodity_graph.py(1 hunks)temoa/model_checking/commodity_network.py(2 hunks)temoa/model_checking/commodity_network_manager.py(1 hunks)temoa/model_checking/element_checker.py(3 hunks)temoa/model_checking/network_model_data.py(1 hunks)temoa/model_checking/pricing_check.py(2 hunks)temoa/model_checking/validators.py(17 hunks)temoa/types/core_types.py(1 hunks)temoa/utilities/graph_utils.py(1 hunks)temoa/utilities/network_vis_templates/graph_script.js(1 hunks)temoa/utilities/network_vis_templates/graph_styles.css(1 hunks)temoa/utilities/network_vis_templates/graph_template.html(1 hunks)temoa/utilities/visualizer.py(1 hunks)tests/test_network_model_data.py(2 hunks)tests/test_source_check.py(1 hunks)
💤 Files with no reviewable changes (1)
- temoa/model_checking/init.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T15:53:41.820Z
Learnt from: ParticularlyPythonicBS
PR: TemoaProject/temoa#177
File: temoa/model_checking/commodity_network.py:26-33
Timestamp: 2025-10-27T15:53:41.820Z
Learning: The Temoa project requires Python 3.12 or above as the minimum supported version, so PEP 695 `type` syntax for type aliases is appropriate and preferred over `TypeAlias`.
Applied to files:
temoa/model_checking/commodity_network.pytemoa/model_checking/commodity_network_manager.py
🧬 Code graph analysis (9)
tests/test_source_check.py (2)
temoa/model_checking/commodity_network.py (2)
CommodityNetwork(36-333)analyze_network(169-228)temoa/model_checking/network_model_data.py (1)
EdgeTuple(28-35)
temoa/utilities/visualizer.py (1)
temoa/utilities/graph_utils.py (1)
convert_graph_to_json(27-91)
temoa/data_io/hybrid_loader.py (2)
temoa/model_checking/element_checker.py (2)
ViableSet(18-157)filter_elements(169-249)temoa/model_checking/network_model_data.py (3)
build(138-138)build(140-140)build(141-144)
temoa/utilities/graph_utils.py (1)
temoa/model_checking/network_model_data.py (1)
EdgeTuple(28-35)
temoa/model_checking/commodity_graph.py (4)
temoa/core/config.py (1)
TemoaConfig(32-261)temoa/model_checking/network_model_data.py (2)
EdgeTuple(28-35)NetworkModelData(77-133)temoa/utilities/graph_utils.py (2)
calculate_initial_positions(130-192)calculate_tech_graph_positions(195-243)temoa/utilities/visualizer.py (2)
make_nx_graph(43-181)nx_to_vis(184-260)
tests/test_network_model_data.py (2)
temoa/model_checking/commodity_network.py (6)
CommodityNetwork(36-333)analyze_network(169-228)get_valid_tech(298-305)get_demand_side_orphans(307-314)get_other_orphans(316-323)unsupported_demands(325-333)temoa/model_checking/network_model_data.py (2)
_build_from_db(324-469)clone(109-111)
temoa/model_checking/commodity_network.py (2)
temoa/model_checking/network_model_data.py (2)
EdgeTuple(28-35)NetworkModelData(77-133)temoa/model_checking/commodity_network_manager.py (1)
analyze_network(108-127)
temoa/model_checking/network_model_data.py (2)
temoa/core/model.py (1)
TemoaModel(97-1112)temoa/extensions/myopic/myopic_index.py (1)
MyopicIndex(37-57)
temoa/model_checking/commodity_network_manager.py (3)
temoa/model_checking/commodity_graph.py (1)
visualize_graph(230-310)temoa/model_checking/element_checker.py (2)
ViableSet(18-157)exception_loc(107-108)temoa/model_checking/network_model_data.py (3)
EdgeTuple(28-35)NetworkModelData(77-133)clone(109-111)
🪛 Ruff (0.14.1)
temoa/utilities/graph_utils.py
27-32: Generic function convert_graph_to_json should use type parameters
Use type parameters
(UP047)
235-235: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
temoa/model_checking/element_checker.py
126-126: Avoid specifying long messages outside the exception class
(TRY003)
192-192: Avoid specifying long messages outside the exception class
(TRY003)
195-197: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_network_model_data.py
166-166: Avoid specifying long messages outside the exception class
(TRY003)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
386-386: Avoid specifying long messages outside the exception class
(TRY003)
temoa/model_checking/network_model_data.py
105-107: Avoid specifying long messages outside the exception class
(TRY003)
258-268: Possible SQL injection vector through string-based query construction
(S608)
270-278: Possible SQL injection vector through string-based query construction
(S608)
temoa/model_checking/commodity_network_manager.py
119-119: Unnecessary sorted() call within set()
Remove the inner sorted() call
(C414)
135-135: Avoid specifying long messages outside the exception class
(TRY003)
203-203: Avoid specifying long messages outside the exception class
(TRY003)
temoa/model_checking/validators.py
112-112: Unused function argument: M
(ARG001)
128-128: Unused function argument: M
(ARG001)
264-264: Unused function argument: val
(ARG001)
380-380: Unused function argument: val
(ARG001)
398-398: Unused function argument: M
(ARG001)
398-398: Unused function argument: args
(ARG001)
🔇 Additional comments (9)
requirements.txt (1)
259-259: Version alignment check.
networkx is pinned to 3.5 here; pyproject has networkx>=3.3 and dev adds types-networkx. Confirm CI tests/lint run with consistent versions to avoid stub/runtime drift.requirements-dev.txt (1)
276-276: Mirror the runtime/stubs check in dev constraints.
Ensure dev env resolves networkx and types-networkx consistently with runtime to prevent editor/type-check drift.temoa/model_checking/validators.py (2)
35-35: Import choices OK; be explicit about Set intent.
Using pyomo.core.Set is fine since you rely on set algebra (&). Document that these are Pyomo Sets, not typing.Set, to avoid confusion.Also applies to: 37-37
145-158: LGTM for these validators and set checks.
Signatures and set/dimension checks are clear and consistent with model indices.Also applies to: 160-178, 199-217, 219-241, 243-261, 278-287, 299-313, 369-376
tests/test_source_check.py (1)
154-213: Solid parametrized coverage; exercises key topologies and API surfaces.Test data construction aligns with EdgeTuple shape and CommodityNetwork expectations; assertions match public sets. LGTM.
temoa/data_io/hybrid_loader.py (2)
341-344: Typed validation path looks correct.Casting to Sequence[tuple[ValidationPrimitive,...]] and delegating to element_checker.filter_elements matches the validator API.
459-469: Evaluate rtt Cartesian product concern for LinkedTech validation against actual model usage patterns.The review comment raises a valid technical concern: constructing
rtt = {(r, t1, t2) for r, t1 in rt_tuples for (t2,) in t_tuples}creates an O(|rt| × |t|) memory footprint at initialization. However, the concern is speculative rather than addressing a reported issue.Key findings:
- LinkedTech table is optional (
is_table_required=False), typically absent or small in many models- The product is constructed once during
_build_efficiency_dataset()initialization, not in hot loops- No performance complaints, TODOs, or production failures visible in the codebase
- No comparable guardrails exist for similar operations (e.g.,
ic_tuples | oc_tuplesunion at line 462)- Threshold of 500K tuples is reasonable but arbitrary without knowing typical model sizes
The code already shows design awareness via the comment: "Using member_tuples here is safer as it's unambiguously typed"—indicating tradeoffs between type safety and memory are already considered.
Recommendation: This requires manual verification of whether LinkedTech is actively used in your workflows and what typical region/tech set sizes are. If LinkedTech is rarely used or involves small datasets, optimization may not be necessary. If used extensively with large models, the suggested approaches (lazy validation, composite checks, guardrails) are sound.
temoa/utilities/visualizer.py (1)
212-260: LGTM: robust template handling and safe file writes.Deep-copied options, narrowed try/except with else, and correct dashes/color propagation look solid.
temoa/model_checking/commodity_network_manager.py (1)
26-28: PEP 695 alias: good with 3.12 baseline.Using
type RegionPeriodKey = ...is correct and preferred here. Based on learnings
| .config-toggle-btn::after { content: '\25BC'; display: inline-block; transition: transform 0.2s ease-in-out; } | ||
| .collapsed .config-toggle-btn::after { transform: rotate(-90deg); } | ||
| #config-container-content { max-height: 40vh; overflow-y: auto; padding: 15px; box-sizing: border-box; background-color: #ffffff; transition: max-height 0.3s ease-in-out, padding 0.3s ease-in-out; } | ||
| .collapsed #config-container-content { max-height: 0; padding-top: 0; padding-bottom: 0; overflow: hidden; } |
There was a problem hiding this comment.
Honor prefers-reduced-motion; disable UI animations for users who opt out.
Rotate/height transitions can cause motion issues. Add a reduced-motion override.
+@media (prefers-reduced-motion: reduce) {
+ .config-toggle-btn::after {
+ transition: none;
+ }
+ #config-container-content {
+ transition: none;
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .config-toggle-btn::after { content: '\25BC'; display: inline-block; transition: transform 0.2s ease-in-out; } | |
| .collapsed .config-toggle-btn::after { transform: rotate(-90deg); } | |
| #config-container-content { max-height: 40vh; overflow-y: auto; padding: 15px; box-sizing: border-box; background-color: #ffffff; transition: max-height 0.3s ease-in-out, padding 0.3s ease-in-out; } | |
| .collapsed #config-container-content { max-height: 0; padding-top: 0; padding-bottom: 0; overflow: hidden; } | |
| .config-toggle-btn::after { content: '\25BC'; display: inline-block; transition: transform 0.2s ease-in-out; } | |
| .collapsed .config-toggle-btn::after { transform: rotate(-90deg); } | |
| #config-container-content { max-height: 40vh; overflow-y: auto; padding: 15px; box-sizing: border-box; background-color: #ffffff; transition: max-height 0.3s ease-in-out, padding 0.3s ease-in-out; } | |
| .collapsed #config-container-content { max-height: 0; padding-top: 0; padding-bottom: 0; overflow: hidden; } | |
| @media (prefers-reduced-motion: reduce) { | |
| .config-toggle-btn::after { | |
| transition: none; | |
| } | |
| #config-container-content { | |
| transition: none; | |
| } | |
| } |
🤖 Prompt for AI Agents
In temoa/utilities/network_vis_templates/graph_styles.css around lines 8–11, add
a prefers-reduced-motion override to disable the rotate and height transitions
for users who opt out of motion: create an @media (prefers-reduced-motion:
reduce) block that sets transition: none (or transition-duration: 0s) for
.config-toggle-btn::after and #config-container-content, and ensure any
transform-based rotation is neutralized (e.g., set transform: none) so the UI
changes occur instantly without animation.
| #mynetwork { width: 100%; flex-grow: 1; min-height: 0; } | ||
| .filter-panel { display: flex; align-items: center; gap: 10px; padding: 8px 15px; background-color: #e9ecef; border-bottom: 1px solid #dee2e6; } | ||
| .filter-panel input[type=text] { flex-grow: 1; padding: 6px 8px; border: 1px solid #ced4da; border-radius: 4px; } | ||
| .filter-panel button { padding: 6px 12px; border-radius: 4px; border: 1px solid #6c757d; background-color: #6c757d; color: white; cursor: pointer; } |
There was a problem hiding this comment.
Add visible keyboard focus styles for interactive controls.
Current buttons/links lack explicit focus-visible styling. Improves accessibility (WCAG 2.4.7).
+/* High-contrast focus ring for keyboard users */
+button:focus-visible,
+.config-toggle-btn:focus-visible,
+#advanced-controls-toggle:focus-visible,
+.sector-toggle-item:focus-within {
+ outline: 2px solid #0d6efd;
+ outline-offset: 2px;
+}Also applies to: 27-29
🤖 Prompt for AI Agents
In temoa/utilities/network_vis_templates/graph_styles.css around lines 15 and
27-29, the interactive controls (buttons/links) currently lack visible
focus-visible styling; add an explicit :focus-visible rule (e.g., outline or
box-shadow and outline-offset) for .filter-panel button and any interactive link
selectors at 27-29 to provide a clear, high-contrast focus indicator while
preserving existing padding, border-radius and background; ensure the focus
style meets contrast requirements, does not shift layout (use outline-offset or
box-shadow), and include :focus (optional) fallback for browsers without
:focus-visible support.
This pull request completely overhauls the commodity network visualization by removing the abandoned gravis dependency and introducing an in house interactive visualization framework built directly with vis.js.
The new visualization offers:
Dual Views: The visualization now features two distinct, toggleable views:
Commodity View: The classic view where commodities are nodes and technologies are grouped into edges.
Technology View: A new, technology-centric view where technologies are the nodes and commodity flows are the edges.
Performance: The visualization is static by default, leveraging pre-computed, sector-based layouts. The physics simulation can be manually enabled by the user for exploration via an "Advanced Controls" panel.
Interactivity:
This PR follows the refactoring and typing in #175
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores