Add network_inspectors.py module in LANfactory#82
Merged
cpaniaguam merged 12 commits intoJul 17, 2026
Conversation
Port network-inspector visualizations to LANfactory using ssms.basic_simulators.simulator, ssms.config.ModelConfigBuilder, ssms.support_utils.kde_class.LogKDE, and LoadTorchMLPInfer.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Changesnetwork_inspectors module and wiring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant InspectionAPI
participant Compute
participant Simulator
participant Plotting
Caller->>InspectionAPI: request likelihood comparison
InspectionAPI->>Compute: build evaluation grid
Compute->>Simulator: generate simulated observations
Simulator-->>Compute: return simulation output
Compute-->>InspectionAPI: return LAN and KDE likelihoods
InspectionAPI->>Plotting: render comparison figure
Plotting-->>Caller: save or display figure
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 7
🤖 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/__init__.py`:
- Around line 7-9: The eager import of network_inspectors at the top of the
__init__.py file forces scikit-learn to be loaded when the lanfactory package is
imported, even though sklearn is not in the project dependencies. Remove the
`from . import network_inspectors` statement on line 7 to prevent the transitive
import of sklearn for users who don't need the inspectors module. Additionally,
remove `"network_inspectors"` from the __all__ list on line 9, or if the module
needs to remain part of the public API, implement lazy loading for it instead of
eager importing.
In `@src/lanfactory/network_inspectors.py`:
- Line 121: The plt.subplots() calls at lines 121, 193, 206, 218, and 233 in
src/lanfactory/network_inspectors.py squeeze the axes when rows equals 1 or cols
equals 1, returning a 1D array instead of a 2D array. This causes the subsequent
2D indexing ax[row_tmp, col_tmp] to fail. Add the squeeze=False parameter to all
five plt.subplots() calls to ensure axes are always returned as a 2D array
regardless of the number of rows or columns, preventing indexing crashes.
- Around line 68-80: The kde_vs_lan_likelihoods function and other public
plotting functions accept parameter_df, model, and torch_mlp_predict with None
defaults but immediately dereference these values throughout the function body
(causing opaque runtime errors). Add early validation checks at the start of
each affected function to verify that these required parameters are not None,
and raise a clear, actionable ValueError or TypeError with a descriptive message
if they are missing, rather than allowing them to fail later with cryptic
attribute or type errors.
- Line 119: The sns.set() call on line 119 uses a hardcoded font_scale=2 value,
which ignores the font_scale parameter that is passed to the function (defined
on line 78). Replace the hardcoded font_scale=2 with the font_scale parameter
variable so that the caller's configuration is respected and takes effect in the
visualization settings.
- Around line 360-367: The code at line 360 calls .shape[0] on an element from
vary_dict, expecting it to be a numpy array, but the default value of vary_dict
uses Python lists instead, which lack the .shape attribute and cause an
AttributeError. Find where vary_dict is defined with its default value and
convert the default from using Python lists to using numpy arrays so that the
.shape[0] access at line 360 works correctly with default parameters.
- Around line 139-153: The input_batch array is initialized with a hardcoded
size of 4000 rows on line 151, but the plot_data array is dynamically sized to
len(config["choices"]) * 1000 rows. When len(config["choices"]) is not equal to
2, this causes a shape mismatch when attempting to assign plot_data to the
input_batch slicing operation on line 152. Replace the hardcoded 4000 value in
the input_batch initialization with len(config["choices"]) * 1000 to make the
row count dynamic and match the actual size of plot_data.
- Around line 14-21: The bare except clause at line 17 catches all exceptions
during the import of LoadTorchMLPInfer, not just ImportError. This masks the
actual error when import fails. Replace the bare except with except ImportError
to only catch import-related errors. Additionally, add a check in the
get_torch_mlp function (around line 59) before using LoadTorchMLPInfer to
validate that the import was successful, and raise an appropriate error if it
was not, rather than allowing a delayed NameError to occur.
🪄 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: 87480f93-721c-46ca-8e09-93545249dc66
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomlsrc/lanfactory/__init__.pysrc/lanfactory/network_inspectors.py
cpaniaguam
requested changes
Jun 23, 2026
cpaniaguam
left a comment
cpaniaguam
left a comment
Collaborator
There was a problem hiding this comment.
This is a great start, thanks @mariama-design! Two things to consider:
-
I think it'd be a good idea to add a new version of the old tutorial -- either in ipynb notebook format or marimo. It'd be a nice way to interactively make sure everything works as it did before.
-
Later we should discuss adding test coverage to these features
Collaborator
There was a problem hiding this comment.
This file should not be included in the PR. Add it to the .gitignore file so Git doesn't track changes to it.
Collaborator
There was a problem hiding this comment.
Consider this suggestion.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
cpaniaguam
self-requested a review
June 29, 2026 14:25
cpaniaguam
requested changes
Jun 29, 2026
cpaniaguam
left a comment
cpaniaguam
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for these @mariama-design . I added a few comments for you to consider. I also created an issue to address the length of network_inspectors.py and how it mixes concerns in #85. Once we modularize it we can add a test suite.
Collaborator
There was a problem hiding this comment.
Consider this suggestion.
Collaborator
There was a problem hiding this comment.
Consider moving the tutorial into notebooks (or basic_tutorial). src should only contain importable package code. After moving, update Path(...) usages so paths resolve from the repo root.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/lanfactory/network_inspectors/api.py (3)
63-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid mutable default argument.
While
vary_dictis only read (not mutated) in this function, using a mutable default is a well-known Python footgun. PreferNonewith a sentinel inside the function body.♻️ Proposed refactor
-def lan_manifold( - parameter_df=None, - vary_dict={"v": [-1.0, -0.75, -0.5, -0.25, 0, 0.25, 0.5, 0.75, 1.0]}, - model="ddm", - torch_mlp_predict=None, - grid=None, - plot=None, -): +def lan_manifold( + parameter_df=None, + vary_dict=None, + model="ddm", + torch_mlp_predict=None, + grid=None, + plot=None, +): + if vary_dict is None: + vary_dict = {"v": [-1.0, -0.75, -0.5, -0.25, 0, 0.25, 0.5, 0.75, 1.0]}🤖 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` at line 63, The default value for vary_dict in the network inspector API function is a mutable dict, which should be avoided. Update the function signature to use None as the default and initialize the existing {"v": [...]} mapping inside the function body when vary_dict is not provided, keeping the logic in the same API entry point and preserving current behavior.
20-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations for mypy compliance.
As per coding guidelines, Python files should pass mypy type checking. Neither
kde_vs_lan_likelihoodsnorlan_manifoldhas type annotations on parameters or return types, making static analysis ineffective.🤖 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 20 - 58, Add explicit type annotations to the public API in this module so mypy can validate it. Update kde_vs_lan_likelihoods to annotate all parameters and its return type, and do the same for lan_manifold if it is defined in this file. Use the existing symbols like ModelSpec, PlotConfig, make_rt_choice_grid, and plot_kde_vs_lan to infer the correct input/output types, and keep the current runtime behavior unchanged.Source: Coding guidelines
89-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
This debug-style
logging.getLogger(__name__).info(...).🤖 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` at line 89, The debug message in the parameter handling path is using print, which will clutter user output. Update the relevant code in the API flow around the first-row selection logic to either remove the message entirely or route it through logging with logging.getLogger(__name__).info(...), so the behavior remains visible without writing directly to stdout.src/lanfactory/network_inspectors/compute.py (1)
13-23: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider
np.arangeinstead of list comprehensions for grid construction.The list comprehensions create Python lists that
np.concatenatemust convert. Usingnp.arangedirectly produces arrays and is both more idiomatic and slightly faster for larger grids.♻️ Suggested refactor
def _symmetric_2choice_grid(n, step): """(rt, choice) grid: n points per side at rt = i*step, choices -1 and +1.""" plot_data = np.zeros((2 * n, 2)) plot_data[:, 0] = np.concatenate( ( - [i * step for i in range(n, 0, -1)], - [i * step for i in range(1, n + 1, 1)], + np.arange(n, 0, -1) * step, + np.arange(1, n + 1) * step, ) ) plot_data[:, 1] = np.concatenate((np.repeat(-1, n), np.repeat(1, n))) return plot_data🤖 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/compute.py` around lines 13 - 23, The grid construction in _symmetric_2choice_grid uses Python list comprehensions inside np.concatenate, which is less idiomatic and adds unnecessary list-to-array conversion. Update the rt value generation to use np.arange directly for the descending and ascending sequences, and keep the existing np.repeat logic for the choice column so the function still returns the same shaped array with clearer, faster array construction.src/lanfactory/network_inspectors/config.py (1)
42-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten
figsizetype annotation for better mypy precision.
figsize: tupleresolves totuple[Any, ...]under mypy. Usingtuple[int, int](ortuple[float, float]) would give callers and downstream code a more precise contract. As per coding guidelines, Python files should pass mypy type checking.♻️ Suggested type tightening
- figsize: tuple = (10, 10) + figsize: tuple[int, int] = (10, 10)🤖 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/config.py` around lines 42 - 53, Tighten the PlotConfig.figsize annotation in the PlotConfig dataclass so mypy sees a concrete pair type instead of a generic tuple[Any, ...]. Update the figsize field in PlotConfig to use a fixed two-item tuple type, and keep the default value compatible with that contract so callers and downstream plotting code get precise type checking.Source: Coding guidelines
🤖 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 87-94: The non-DataFrame path in lan_manifold is broken because
parameter_df.iloc[0, :] is executed before checking whether parameter_df is
actually a pandas DataFrame, so numpy array inputs fail and the fallback branch
is unreachable for valid non-empty vectors. Update the flow in lan_manifold to
branch on isinstance(parameter_df, pd.DataFrame) first, then either select and
squeeze the requested spec.params columns for DataFrames or pass through the
provided vector unchanged for non-DataFrame inputs. Keep the first-row handling
and print message only in the DataFrame path so build_manifold receives the
correct parameters for both input types.
In `@src/lanfactory/network_inspectors/compute.py`:
- Around line 48-59: Guard evaluate_network against a missing
ModelSpec.predictor before calling it. Since ModelSpec.predictor can be None and
evaluate_network is public, add an explicit check in evaluate_network and raise
a clear, intentional error message instead of allowing the NoneType callable
failure. Use the existing evaluate_network and spec.predictor symbols so the fix
is localized and easy to find.
- Around line 79-82: The build_manifold function currently relies on
spec.params.index(vary_name), which throws a vague ValueError when vary_name is
not a valid parameter. Add an explicit validation step in build_manifold before
using the index lookup, and raise a clearer error that names the invalid
vary_name and lists the valid spec.params entries so callers can immediately see
what parameter names are allowed.
In `@src/lanfactory/network_inspectors/plotting.py`:
- Around line 13-18: The directory creation in _save_figure is racy and only
handles a single missing level. Replace the os.path.isdir/os.mkdir branch with a
direct os.makedirs call using exist_ok=True on cfg.save_dir, and keep the
plt.savefig path construction unchanged so nested output directories are created
safely.
- Around line 176-180: `plot_manifold` leaves the figure open when `cfg.show` is
true, unlike `plot_kde_vs_lan`, which can leak figures on repeated calls. Update
the `plot_manifold` branch that calls `plt.show()` so it also closes the current
figure afterward, keeping the cleanup behavior consistent with the non-show path
in the same function.
- Around line 61-71: The multi-choice plotting slices in plotting.py are
hardcoded to 1000, which breaks alignment when the grid size comes from
make_rt_choice_grid via grid_spec.n_points_mc. Update the slicing logic in the
multi-choice branch of the plotting routine to use the actual points-per-choice
value from the grid/spec instead of a fixed constant, and apply the same change
in the LAN likelihood multi-choice branch as well. Reference the plotting code
around the sns.lineplot loop so the fix stays consistent across both branches.
---
Nitpick comments:
In `@src/lanfactory/network_inspectors/api.py`:
- Line 63: The default value for vary_dict in the network inspector API function
is a mutable dict, which should be avoided. Update the function signature to use
None as the default and initialize the existing {"v": [...]} mapping inside the
function body when vary_dict is not provided, keeping the logic in the same API
entry point and preserving current behavior.
- Around line 20-58: Add explicit type annotations to the public API in this
module so mypy can validate it. Update kde_vs_lan_likelihoods to annotate all
parameters and its return type, and do the same for lan_manifold if it is
defined in this file. Use the existing symbols like ModelSpec, PlotConfig,
make_rt_choice_grid, and plot_kde_vs_lan to infer the correct input/output
types, and keep the current runtime behavior unchanged.
- Line 89: The debug message in the parameter handling path is using print,
which will clutter user output. Update the relevant code in the API flow around
the first-row selection logic to either remove the message entirely or route it
through logging with logging.getLogger(__name__).info(...), so the behavior
remains visible without writing directly to stdout.
In `@src/lanfactory/network_inspectors/compute.py`:
- Around line 13-23: The grid construction in _symmetric_2choice_grid uses
Python list comprehensions inside np.concatenate, which is less idiomatic and
adds unnecessary list-to-array conversion. Update the rt value generation to use
np.arange directly for the descending and ascending sequences, and keep the
existing np.repeat logic for the choice column so the function still returns the
same shaped array with clearer, faster array construction.
In `@src/lanfactory/network_inspectors/config.py`:
- Around line 42-53: Tighten the PlotConfig.figsize annotation in the PlotConfig
dataclass so mypy sees a concrete pair type instead of a generic tuple[Any,
...]. Update the figsize field in PlotConfig to use a fixed two-item tuple type,
and keep the default value compatible with that contract so callers and
downstream plotting code get precise type checking.
🪄 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: 2b149220-ed8a-4aa5-b1f5-067ce9c7f188
📒 Files selected for processing (7)
src/lanfactory/network_inspectors/__init__.pysrc/lanfactory/network_inspectors/api.pysrc/lanfactory/network_inspectors/compute.pysrc/lanfactory/network_inspectors/config.pysrc/lanfactory/network_inspectors/loaders.pysrc/lanfactory/network_inspectors/plotting.pysrc/network_inspectors_tutorial.ipynb
Collaborator
|
@mariama-design I added two changes so changes to the uv.lock file are not tracked. You might want to do |
mariama-design
pushed a commit
to mariama-design/LANfactory
that referenced
this pull request
Jul 11, 2026
- Apply remaining review fixes to the package: assert -> ValueError for the 2-choice constraint, print -> logging, integer division for subplot rows, np.arange RT grids, and require a pandas.DataFrame in kde_vs_lan_likelihoods - Remove the dead single-file src/lanfactory/network_inspectors.py, now fully replaced by the network_inspectors/ package (Python shadowed it anyway) - Move the tutorial notebook out of src/ into notebooks/ Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Apply remaining review fixes to the package: assert -> ValueError for the 2-choice constraint, print -> logging, integer division for subplot rows, np.arange RT grids, and require a pandas.DataFrame in kde_vs_lan_likelihoods - Remove the dead single-file src/lanfactory/network_inspectors.py, now fully replaced by the network_inspectors/ package (Python shadowed it anyway) - Move the tutorial notebook out of src/ into notebooks/
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
mariama-design
force-pushed
the
add-network-inspectors
branch
from
July 11, 2026 03:26
640e74d to
f1ebd8a
Compare
cpaniaguam
requested changes
Jul 13, 2026
cpaniaguam
previously approved these changes
Jul 16, 2026
cpaniaguam
left a comment
cpaniaguam
left a comment
Collaborator
There was a problem hiding this comment.
@mariama-design great work, thanks! Feel free to merge the PR.
mariama-design
force-pushed
the
add-network-inspectors
branch
from
July 16, 2026 20:15
5ba375b to
d5c5824
Compare
cpaniaguam
self-requested a review
July 17, 2026 20:53
cpaniaguam
approved these changes
Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port network-inspector visualizations to LANfactory using
ssms.basic_simulators.simulator,
ssms.config.ModelConfigBuilder, ssms.support_utils.kde_class.LogKDE, and
LoadTorchMLPInfer.
Summary by CodeRabbit
Release Notes
seabornto main dependencies for improved plotting support..gitignoreto excludeuv.lock.