Issue 97 interactive manifold#98
Closed
mariama-design wants to merge 13 commits into
Closed
Conversation
Port network-inspector visualizations to LANfactory using ssms.basic_simulators.simulator, ssms.config.ModelConfigBuilder, ssms.support_utils.kde_class.LogKDE, and LoadTorchMLPInfer.
- 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/
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
📝 WalkthroughWalkthroughChangesIntroduces a public Network inspection workflows
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Notebook
participant lanfactory.network_inspectors
participant TorchMLP
participant ssms
participant Plotting
Notebook->>lanfactory.network_inspectors: construct LAN predictor
lanfactory.network_inspectors->>TorchMLP: load predict_on_batch
Notebook->>lanfactory.network_inspectors: request KDE/LAN comparison
lanfactory.network_inspectors->>TorchMLP: evaluate LAN likelihoods
lanfactory.network_inspectors->>ssms: simulate ground-truth samples
lanfactory.network_inspectors->>Plotting: render comparison or manifold
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 5
🧹 Nitpick comments (2)
tests/test_network_inspectors_plotting.py (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the surface coordinates and likelihood matrix.
The current assertions still pass if signed-RT ordering or the pivoted likelihood values are wrong.
Proposed assertions
+ assert list(fig.data[0].x) == [-2.0, -1.0, 1.0, 2.0] + assert list(fig.data[0].y) == [0.1, 0.2] + assert fig.data[0].z.tolist() == [ + [0.2, 0.1, 0.3, 0.4], + [0.3, 0.2, 0.4, 0.5], + ] assert (tmp_path / "mlp_manifold_ddm.html").exists()🤖 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 `@tests/test_network_inspectors_plotting.py` around lines 25 - 29, Strengthen the test around plot_manifold by asserting the surface trace’s x/y coordinates match the expected signed-RT ordering and its z values match the expected pivoted likelihood matrix. Keep the existing Figure, surface-type, and HTML-output assertions, using the test’s known manifold/spec data to verify exact coordinate and likelihood values.src/lanfactory/network_inspectors/compute.py (1)
63-68: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAllocate
input_batchas float32 to avoid redundant array copying.Since
spec.predictorexpects afloat32matrix, you can avoid allocating a fullfloat64grid and then duplicating it via.astype(np.float32)by creating the initial zeros array withdtype=np.float32. Thegridassignment will automatically cast fromfloat64tofloat32.♻️ Proposed refactor
- input_batch = np.zeros((grid.shape[0], spec.n_params + 2)) + input_batch = np.zeros((grid.shape[0], spec.n_params + 2), dtype=np.float32) input_batch[:, : spec.n_params] = params input_batch[:, spec.n_params :] = grid - ll_out = spec.predictor(input_batch.astype(np.float32)) + ll_out = spec.predictor(input_batch)🤖 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 63 - 68, Update the input_batch allocation in the predictor preparation flow to use np.float32 directly, so the later input_batch.astype(np.float32) conversion does not create a redundant copy. Preserve the existing parameter and grid assignments and predictor invocation.
🤖 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 `@notebooks/network_inspectors_tutorial.ipynb`:
- Around line 100-109: Update the network_input initialization flow to populate
network_input[:, :-2] with the parameter_vector prepared in the preceding cell
before invoking the LAN, while preserving the existing reaction-time and choice
assignments in the final two columns.
In `@src/lanfactory/network_inspectors/api.py`:
- Around line 92-93: Validate vary_dict at the start of the parameter-sweep
logic so it contains exactly one key with a non-empty sweep before accessing its
contents. Preserve the existing default assignment for None, but reject empty
dictionaries and mappings with multiple keys rather than allowing IndexError or
silently ignoring parameters; update the related handling around the
parameter-sweep code near the referenced logic.
- Around line 51-56: Update the validation in the API function containing
ModelSpec.from_model to reject empty parameter_df values before calling
make_rt_choice_grid or constructing the subplot grid. Raise a clear ValueError
for an empty DataFrame while preserving the existing TypeError for non-DataFrame
inputs and normal behavior for non-empty frames.
- Around line 103-110: Update the non-DataFrame branch handling parameter_df so
NumPy inputs are normalized to a one-dimensional float32 parameter vector before
build_manifold is called. Apply the same first-row behavior used for DataFrames
when the array is two-dimensional, and validate the resulting shape before
manifold construction.
- Around line 54-68: Update the parameter extraction in the loop around
evaluate_network and simulate_ground_truth to select columns by the model’s
parameter names and arrange them in the model’s expected order, rather than
using every DataFrame column positionally. Exclude unrelated columns and
preserve the resulting ordered values for both network evaluation and
ground-truth simulation.
---
Nitpick comments:
In `@src/lanfactory/network_inspectors/compute.py`:
- Around line 63-68: Update the input_batch allocation in the predictor
preparation flow to use np.float32 directly, so the later
input_batch.astype(np.float32) conversion does not create a redundant copy.
Preserve the existing parameter and grid assignments and predictor invocation.
In `@tests/test_network_inspectors_plotting.py`:
- Around line 25-29: Strengthen the test around plot_manifold by asserting the
surface trace’s x/y coordinates match the expected signed-RT ordering and its z
values match the expected pivoted likelihood matrix. Keep the existing Figure,
surface-type, and HTML-output assertions, using the test’s known manifold/spec
data to verify exact coordinate and likelihood values.
🪄 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: 9f2919d6-8966-4b2d-99c5-a9cfe00e00a7
📒 Files selected for processing (11)
.gitignorenotebooks/network_inspectors_tutorial.ipynbpyproject.tomlsrc/lanfactory/__init__.pysrc/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.pytests/test_network_inspectors_plotting.py
Comment on lines
+100
to
+109
| "# Initialize network input\n", | ||
| "network_input: NDArray[np.float32] = np.zeros(\n", | ||
| " (parameter_matrix.shape[0], parameter_matrix.shape[1] + 2), dtype=np.float32\n", | ||
| ")\n", | ||
| "\n", | ||
| "# Add reaction times\n", | ||
| "network_input[:, -2] = np.linspace(0, 3, parameter_matrix.shape[0])\n", | ||
| "\n", | ||
| "# Add choices\n", | ||
| "network_input[:, -1] = np.repeat(np.random.choice([-1, 1]), parameter_matrix.shape[0])\n", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Populate the model-parameter columns before invoking the LAN.
network_input[:, :-2] remains all zeros, so the displayed outputs do not correspond to the parameter_vector prepared in the preceding cell.
network_input: NDArray[np.float32] = np.zeros(
(parameter_matrix.shape[0], parameter_matrix.shape[1] + 2), dtype=np.float32
)
+network_input[:, :-2] = parameter_matrix📝 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.
Suggested change
| "# Initialize network input\n", | |
| "network_input: NDArray[np.float32] = np.zeros(\n", | |
| " (parameter_matrix.shape[0], parameter_matrix.shape[1] + 2), dtype=np.float32\n", | |
| ")\n", | |
| "\n", | |
| "# Add reaction times\n", | |
| "network_input[:, -2] = np.linspace(0, 3, parameter_matrix.shape[0])\n", | |
| "\n", | |
| "# Add choices\n", | |
| "network_input[:, -1] = np.repeat(np.random.choice([-1, 1]), parameter_matrix.shape[0])\n", | |
| "# Initialize network input\n", | |
| "network_input: NDArray[np.float32] = np.zeros(\n", | |
| " (parameter_matrix.shape[0], parameter_matrix.shape[1] + 2), dtype=np.float32\n", | |
| ")\n", | |
| "network_input[:, :-2] = parameter_matrix\n", | |
| "\n", | |
| "# Add reaction times\n", | |
| "network_input[:, -2] = np.linspace(0, 3, parameter_matrix.shape[0])\n", | |
| "\n", | |
| "# Add choices\n", | |
| "network_input[:, -1] = np.repeat(np.random.choice([-1, 1]), parameter_matrix.shape[0])\n", |
🤖 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 `@notebooks/network_inspectors_tutorial.ipynb` around lines 100 - 109, Update
the network_input initialization flow to populate network_input[:, :-2] with the
parameter_vector prepared in the preceding cell before invoking the LAN, while
preserving the existing reaction-time and choice assignments in the final two
columns.
Comment on lines
+51
to
+56
| if not isinstance(parameter_df, pd.DataFrame): | ||
| raise TypeError("parameter_df must be a pandas.DataFrame.") | ||
|
|
||
| spec = ModelSpec.from_model(model, predictor=torch_mlp_predict) | ||
| cfg = plot or PlotConfig() | ||
| grid_arr = make_rt_choice_grid(spec, grid) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject empty parameter frames before constructing the subplot grid.
An empty DataFrame produces results=[]; plot_kde_vs_lan then requests zero subplot rows and raises an opaque Matplotlib exception.
if not isinstance(parameter_df, pd.DataFrame):
raise TypeError("parameter_df must be a pandas.DataFrame.")
+ if parameter_df.empty:
+ raise ValueError("parameter_df must contain at least one parameter vector.")📝 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.
Suggested change
| if not isinstance(parameter_df, pd.DataFrame): | |
| raise TypeError("parameter_df must be a pandas.DataFrame.") | |
| spec = ModelSpec.from_model(model, predictor=torch_mlp_predict) | |
| cfg = plot or PlotConfig() | |
| grid_arr = make_rt_choice_grid(spec, grid) | |
| if not isinstance(parameter_df, pd.DataFrame): | |
| raise TypeError("parameter_df must be a pandas.DataFrame.") | |
| if parameter_df.empty: | |
| raise ValueError("parameter_df must contain at least one parameter vector.") | |
| spec = ModelSpec.from_model(model, predictor=torch_mlp_predict) | |
| cfg = plot or PlotConfig() | |
| grid_arr = make_rt_choice_grid(spec, grid) |
🤖 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 51 - 56, Update the
validation in the API function containing ModelSpec.from_model to reject empty
parameter_df values before calling make_rt_choice_grid or constructing the
subplot grid. Raise a clear ValueError for an empty DataFrame while preserving
the existing TypeError for non-DataFrame inputs and normal behavior for
non-empty frames.
Comment on lines
+54
to
+68
| spec = ModelSpec.from_model(model, predictor=torch_mlp_predict) | ||
| cfg = plot or PlotConfig() | ||
| grid_arr = make_rt_choice_grid(spec, grid) | ||
|
|
||
| results: list[LikelihoodResult] = [] | ||
| for i in range(parameter_df.shape[0]): | ||
| params = parameter_df.iloc[i, :].values | ||
| lan_like = np.exp(evaluate_network(spec, params, grid_arr)) | ||
| kdes = [ | ||
| np.exp( | ||
| evaluate_kde(simulate_ground_truth(spec, params, n_samples), grid_arr) | ||
| ) | ||
| for _ in range(n_reps) | ||
| ] | ||
| results.append({"lan": lan_like, "kdes": kdes}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Select model parameters by name and in model order.
Line 60 passes every DataFrame column in its current order. Reordered or extra columns therefore produce invalid network inputs or silently associate values with the wrong model parameters.
Proposed fix
spec = ModelSpec.from_model(model, predictor=torch_mlp_predict)
+ missing_params = [
+ parameter for parameter in spec.params if parameter not in parameter_df.columns
+ ]
+ if missing_params:
+ raise ValueError(f"Missing model parameters: {missing_params}")
+
+ parameter_matrix = parameter_df.loc[:, spec.params].to_numpy(dtype=np.float32)
cfg = plot or PlotConfig()
grid_arr = make_rt_choice_grid(spec, grid)
results: list[LikelihoodResult] = []
- for i in range(parameter_df.shape[0]):
- params = parameter_df.iloc[i, :].values
+ for params in parameter_matrix:📝 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.
Suggested change
| spec = ModelSpec.from_model(model, predictor=torch_mlp_predict) | |
| cfg = plot or PlotConfig() | |
| grid_arr = make_rt_choice_grid(spec, grid) | |
| results: list[LikelihoodResult] = [] | |
| for i in range(parameter_df.shape[0]): | |
| params = parameter_df.iloc[i, :].values | |
| lan_like = np.exp(evaluate_network(spec, params, grid_arr)) | |
| kdes = [ | |
| np.exp( | |
| evaluate_kde(simulate_ground_truth(spec, params, n_samples), grid_arr) | |
| ) | |
| for _ in range(n_reps) | |
| ] | |
| results.append({"lan": lan_like, "kdes": kdes}) | |
| spec = ModelSpec.from_model(model, predictor=torch_mlp_predict) | |
| missing_params = [ | |
| parameter for parameter in spec.params if parameter not in parameter_df.columns | |
| ] | |
| if missing_params: | |
| raise ValueError(f"Missing model parameters: {missing_params}") | |
| parameter_matrix = parameter_df.loc[:, spec.params].to_numpy(dtype=np.float32) | |
| cfg = plot or PlotConfig() | |
| grid_arr = make_rt_choice_grid(spec, grid) | |
| results: list[LikelihoodResult] = [] | |
| for params in parameter_matrix: | |
| lan_like = np.exp(evaluate_network(spec, params, grid_arr)) | |
| kdes = [ | |
| np.exp( | |
| evaluate_kde(simulate_ground_truth(spec, params, n_samples), grid_arr) | |
| ) | |
| for _ in range(n_reps) | |
| ] | |
| results.append({"lan": lan_like, "kdes": kdes}) |
🤖 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 54 - 68, Update the
parameter extraction in the loop around evaluate_network and
simulate_ground_truth to select columns by the model’s parameter names and
arrange them in the model’s expected order, rather than using every DataFrame
column positionally. Exclude unrelated columns and preserve the resulting
ordered values for both network evaluation and ground-truth simulation.
Comment on lines
+92
to
+93
| 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]} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require exactly one non-empty parameter sweep.
An empty dictionary raises IndexError, while multiple keys are silently ignored despite the API promising one swept parameter.
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]}
+ if len(vary_dict) != 1:
+ raise ValueError("vary_dict must contain exactly one parameter.")
- vary_name = list(vary_dict.keys())[0]
- vary_values = np.asarray(vary_dict[vary_name])
+ vary_name, raw_values = next(iter(vary_dict.items()))
+ vary_values = np.asarray(raw_values)
+ if vary_values.size == 0:
+ raise ValueError("The parameter sweep must contain at least one value.")Also applies to: 112-116
🤖 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 92 - 93, Validate
vary_dict at the start of the parameter-sweep logic so it contains exactly one
key with a non-empty sweep before accessing its contents. Preserve the existing
default assignment for None, but reject empty dictionaries and mappings with
multiple keys rather than allowing IndexError or silently ignoring parameters;
update the related handling around the parameter-sweep code near the referenced
logic.
Comment on lines
+103
to
+110
| if isinstance(parameter_df, pd.DataFrame): | ||
| if parameter_df.shape[0] > 0: | ||
| logger.info("Using only the first row of the supplied parameter array.") | ||
| parameters = np.squeeze( | ||
| parameter_df.iloc[0, :][spec.params].values.astype(np.float32) | ||
| ) | ||
| else: | ||
| parameters = np.asarray(parameter_df, dtype=np.float32) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize and validate NumPy parameter arrays before building the manifold.
The annotation permits a two-dimensional array, but only DataFrames receive first-row handling. Passing np.array([[...]]) leaves parameters two-dimensional, causing build_manifold to mutate the wrong axis or fail.
Proposed fix
else:
parameters = np.asarray(parameter_df, dtype=np.float32)
+ if parameters.ndim == 2:
+ if parameters.shape[0] == 0:
+ raise ValueError("parameter_df must contain at least one row.")
+ parameters = parameters[0]
+ if parameters.ndim != 1 or parameters.size != spec.n_params:
+ raise ValueError(
+ f"Expected one parameter vector with {spec.n_params} values."
+ )📝 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.
Suggested change
| if isinstance(parameter_df, pd.DataFrame): | |
| if parameter_df.shape[0] > 0: | |
| logger.info("Using only the first row of the supplied parameter array.") | |
| parameters = np.squeeze( | |
| parameter_df.iloc[0, :][spec.params].values.astype(np.float32) | |
| ) | |
| else: | |
| parameters = np.asarray(parameter_df, dtype=np.float32) | |
| if isinstance(parameter_df, pd.DataFrame): | |
| if parameter_df.shape[0] > 0: | |
| logger.info("Using only the first row of the supplied parameter array.") | |
| parameters = np.squeeze( | |
| parameter_df.iloc[0, :][spec.params].values.astype(np.float32) | |
| ) | |
| else: | |
| parameters = np.asarray(parameter_df, dtype=np.float32) | |
| if parameters.ndim == 2: | |
| if parameters.shape[0] == 0: | |
| raise ValueError("parameter_df must contain at least one row.") | |
| parameters = parameters[0] | |
| if parameters.ndim != 1 or parameters.size != spec.n_params: | |
| raise ValueError( | |
| f"Expected one parameter vector with {spec.n_params} values." | |
| ) |
🤖 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 103 - 110, Update the
non-DataFrame branch handling parameter_df so NumPy inputs are normalized to a
one-dimensional float32 parameter vector before build_manifold is called. Apply
the same first-row behavior used for DataFrames when the array is
two-dimensional, and validate the resulting shape before manifold construction.
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.
Summary by CodeRabbit