Add interactive rendering for network inspector manifold plots#99
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
📝 WalkthroughWalkthroughNetwork inspector APIs now validate parameter inputs and ordering explicitly. Manifold plots use interactive Plotly surfaces with optional HTML export, supported by a dependency update, tutorial input preparation, and pytest coverage. ChangesNetwork inspector updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant lan_manifold
participant plot_manifold
participant Plotly
lan_manifold->>plot_manifold: pass manifold data and varying parameter
plot_manifold->>Plotly: construct interactive surface figure
plot_manifold->>Plotly: export HTML when configured
Plotly-->>lan_manifold: return go.Figure
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
src/lanfactory/network_inspectors/plotting.py (1)
157-198: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPivot/sort/surface construction verified correct against test expectations.
Traced
signed_rt, pivot, and double-sort logic againsttest_plot_manifold_returns_interactive_plotly_figurevalues —x/y/zoutputs match exactly.One consideration:
include_plotlyjs="cdn"(Line 192) requires network access to render the saved HTML plotly.js bundle offline. If the exported HTML is meant to be viewable without internet access (e.g., shared/archived), considerinclude_plotlyjs=Trueto embed the full library at the cost of file size.♻️ Optional: embed plotly.js for offline viewing
fig.write_html( os.path.join(cfg.save_dir, "mlp_manifold_" + spec.name + ".html"), - include_plotlyjs="cdn", + include_plotlyjs=True, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lanfactory/network_inspectors/plotting.py` around lines 157 - 198, Update the save branch in the manifold plotting function around fig.write_html to embed Plotly.js with include_plotlyjs=True, ensuring exported HTML files render without network access while preserving the existing output path and plotting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lanfactory/network_inspectors/api.py`:
- Around line 119-129: Update the row-count guard before the “Using only the
first row” log to check whether parameter_df contains more than one row, while
preserving the existing empty validation and first-row parameter selection.
---
Nitpick comments:
In `@src/lanfactory/network_inspectors/plotting.py`:
- Around line 157-198: Update the save branch in the manifold plotting function
around fig.write_html to embed Plotly.js with include_plotlyjs=True, ensuring
exported HTML files render without network access while preserving the existing
output path and plotting behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e5dc09c0-3bb8-4d1a-8538-41af4d24b71c
📒 Files selected for processing (6)
notebooks/network_inspectors_tutorial.ipynbpyproject.tomlsrc/lanfactory/network_inspectors/api.pysrc/lanfactory/network_inspectors/plotting.pytests/test_network_inspectors_api.pytests/test_network_inspectors_plotting.py
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the lanfactory.network_inspectors tooling by switching the LAN manifold visualization from a static Matplotlib surface to an interactive Plotly 3D surface, and by surfacing that interactive figure through the manifold API for easier downstream use (display/export).
Changes:
- Replaced Matplotlib-based manifold rendering with an interactive Plotly
Surfaceand HTML export. - Updated the network inspector API to validate parameter inputs more strictly and to return the Plotly figure from
lan_manifold. - Added focused tests covering new validation behavior and the interactive manifold surface output.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/lanfactory/network_inspectors/plotting.py |
Reworks plot_manifold to produce and optionally save an interactive Plotly 3D surface. |
src/lanfactory/network_inspectors/api.py |
Adds parameter validation and returns the Plotly figure from lan_manifold; enforces model parameter column ordering. |
tests/test_network_inspectors_plotting.py |
New test asserting Plotly surface structure and HTML output for the manifold renderer. |
tests/test_network_inspectors_api.py |
New tests for validation of empty/mis-shaped inputs and correct parameter ordering/normalization. |
pyproject.toml |
Adds Plotly as a runtime dependency to support interactive rendering. |
notebooks/network_inspectors_tutorial.ipynb |
Updates tutorial code snippet for revised network input preparation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| width=int(900 * cfg.fig_scale), | ||
| height=int(620 * cfg.fig_scale), | ||
| scene={ | ||
| "xaxis_title": "RT", |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lanfactory/network_inspectors/api.py (1)
102-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a 1D sweep array here.
np.asarraywill let scalar or nested inputs through this size check, butbuild_manifold()iterates overvary_valuesdirectly. A 0-D scalar sweep will fail at iteration time, and higher-rank inputs can produce ambiguous assignments. Usevary_values.ndim == 1(ornp.atleast_1dif scalars are intended).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lanfactory/network_inspectors/api.py` around lines 102 - 108, Update the vary_values validation in the build_manifold sweep setup to require a one-dimensional array before checking or using its contents. Reject scalar and higher-rank inputs with an appropriate ValueError, while preserving the existing non-empty requirement for valid 1D sweeps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lanfactory/network_inspectors/api.py`:
- Around line 102-108: Update the vary_values validation in the build_manifold
sweep setup to require a one-dimensional array before checking or using its
contents. Reject scalar and higher-rank inputs with an appropriate ValueError,
while preserving the existing non-empty requirement for valid 1D sweeps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 306ae530-4587-4ca4-942a-6233be2d9444
📒 Files selected for processing (2)
src/lanfactory/network_inspectors/api.pysrc/lanfactory/network_inspectors/plotting.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lanfactory/network_inspectors/plotting.py
cpaniaguam
left a comment
cpaniaguam
left a comment
There was a problem hiding this comment.
Looks good to me! @mariama-design feel free to click the merge button, thanks!
Summary
Tests
Summary by CodeRabbit