Implement Hopfield node and MNIST demo#14
Conversation
| from fabricpc.core.inference import InferenceSGD | ||
| from fabricpc.core.initializers import XavierInitializer | ||
| from fabricpc.training import train_pcn, evaluate_pcn | ||
| from fabricpc.utils.data.binarized_mnist import load_binarized_mnist |
There was a problem hiding this comment.
ModuleNotFoundError: No module named 'fabricpc.utils.data.binarized_mnist'
Add this file to version control.
matthewbehrend
left a comment
There was a problem hiding this comment.
The utility functions (hebbian_weight_matrix, pseudoinverse_weight_matrix, build_hopfield_graph, recall, pattern_overlap, add_noise) are well-written and useful. The issue is that they're disconnected from the node class they're supposed to support, and the demo doesn't exercise them. The PR would be significantly stronger with a real associative memory demo that validates the recall pipeline end-to-end, and a node class that's either a thin subclass of Linear or genuinely distinct in its forward dynamics.
| # ============================================================================= | ||
|
|
||
|
|
||
| class HopfieldNode(FlattenInputMixin, NodeBase): |
There was a problem hiding this comment.
HopfieldNode.forward() is a copy-paste of Linear.forward() — subclass instead
The entire forward() method is identical to the flatten_input=True path of Linear.forward(). The only actual differences between HopfieldNode and Linear are constructor defaults (TanhActivation, use_bias=False). Same story for initialize_params() — it's Linear.initialize_params() minus the flatten_input branching and the edge key validation.
Requested change: HopfieldNode should subclass Linear and only override init to set Hopfield-appropriate defaults. Delete the duplicated get_slots(), initialize_params(), and forward(). If there's a reason these need to diverge in the future, document that reason now and add a # TODO — don't preemptively fork the code.
| return NodeParams(weights=weights_dict, biases=biases) | ||
|
|
||
| @staticmethod | ||
| def forward( |
There was a problem hiding this comment.
The actual Hopfield logic — Hebbian/pseudoinverse weight storage, recurrent recall dynamics — lives entirely in module-level free functions (store_patterns, recall, build_hopfield_graph, etc.).
This means the "Hopfield node" is really a "Hopfield recipe" — a collection of utility functions that configure a standard Linear node in a specific graph topology. This is can work as a proof of concept, though the goal is a self-contained node type.
| return W | ||
|
|
||
|
|
||
| def store_patterns( |
There was a problem hiding this comment.
store_patterns() bypasses the framework's weight-update mechanism
This function directly constructs a new GraphParams with overwritten weight matrices. In FabricPC, weight updates are supposed to flow through forward_learning() → optimizer. Directly replacing params means:
- A HopfieldNode with Hebbian-stored weights can't participate in end-to-end gradient training alongside other nodes in a larger network.
- There's no way to fine-tune stored patterns.
- It breaks the contract that the training loop owns parameter updates.
This is acceptable as a standalone demo utility, but it should be clearly documented as a non-differentiable initialization step, not presented as the primary way to use the node.
Requested change: Use a Hopfield energy function to learn weights or override the forward_learning method to compute a weight gradient. The system has to learn incrementally in minibatches. The Penrose inverse operation will not see the whole dataset.
| Architecture: | ||
| input(784) ──> HopfieldNode(256, tanh) ──> Linear(10, softmax, CE) | ||
|
|
||
| The HopfieldNode provides the same tanh-based dynamics used in |
There was a problem hiding this comment.
The demo doesn't demonstrate Hopfield behavior
The demo builds input(784) → HopfieldNode(256) → Linear(10) and trains it with standard PC gradient descent. It never calls store_patterns(), recall(), build_hopfield_graph(), or any of the Hopfield-specific machinery from the module.
Requested change:
Write a demo that demonstrates associative memory — pattern storage, noisy recall, convergence — using build_hopfield_graph() and store_patterns()
| query = query[None, :] | ||
| squeeze = True | ||
|
|
||
| clamps = {"input": query} |
There was a problem hiding this comment.
Hardcoded name in _recall_core — composability bug
clamps = {"input": query}
This hardcodes the string "input" regardless of what memory_node_name or any other parameter says. If someone uses build_hopfield_graph(input_name="probe"), recall will silently clamp the wrong node (or fail). The function accepts memory_node_name and feedback_node_name parameters but has no input_node_name parameter — the one name that's actually used is the one that's hardcoded.
Requested change: Add an input_node_name parameter (defaulting to "input") and use it:
clamps = {input_node_name: query}
| # ========================================================================= | ||
|
|
||
|
|
||
| class HopfieldMnistLoader: |
There was a problem hiding this comment.
Custom data loader in the demo duplicates existing infrastructure
HopfieldMnistLoader is a hand-rolled data loader that does what MnistLoader already does. The existing mnist_demo.py uses MnistLoader from fabricpc.utils.data.dataloader.
Requested change: Use or extend MnistLoader to a binary variant. Example files should demonstrate how to use the framework's existing tools, where possible.
| out_shape = node_info.shape | ||
|
|
||
| # Linear combination of all inputs | ||
| pre_activation = FlattenInputMixin.compute_linear( |
There was a problem hiding this comment.
flatten_input is not configurable
HopfieldNode.forward() always takes the flatten path via compute_linear(), but flatten_input is never exposed as a constructor parameter or passed through extra_config. This means:
- Multi-dimensional inputs are silently flattened with no way to opt out.
- The behavior diverges from Linear without documentation.
This will matter when someone tries to use HopfieldNode with sequence or spatial data.
Requested change: expose flatten_input as a parameter (like Linear does); see previous comment to extend Linear rather than duplicate.
Summary
HopfieldNodewith tanh activation, Hebbian and pseudo-inverse storage rules, and PC-based recall with energy trackinghopfield_demo.pythat trains a HopfieldNode network on binarized MNIST with supervised cross-entropy, achieving 98%+ test accuracyTest plan
python examples/hopfield_demo.pyand verify training accuracy reaches ~99%, test accuracy ~98% and train energy ~0.0047