Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the NeuralNet subsystem and related consumers/tests to address PHPStan findings and ongoing refactors (notably around Network/FeedForward types and NDArray usage), plus a few targeted robustness tweaks.
Changes:
- Refactors NeuralNet “Network” usage: introduces
Networks\Base\Contracts\Network, updates snapshots/tests to useNetworks\FeedForward\FeedForward, and removes the oldNetworks\Networkclass. - Updates several learners (classifiers/regressors) and tests to instantiate
FeedForwardinstead ofNetwork. - Adds a handful of correctness/robustness adjustments (RBX checksum parsing validation, CommitteeMachine empty-experts guard, doc/type tweaks, and snapshot expectation updates).
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/NeuralNet/Snapshots/SnapshotTest.php | Updates snapshot tests to use the new Networks\FeedForward\FeedForward and contract imports. |
| tests/NeuralNet/SnapshotTest.php | Switches to constructing NeuralNet\FeedForward in snapshot test. |
| tests/NeuralNet/Networks/NetworkTest.php | Updates imports/instantiation to test the new Networks\FeedForward\FeedForward against the new Networks\Base\Contracts\Network. |
| tests/NeuralNet/NetworkTest.php | Switches instantiation to NeuralNet\FeedForward (legacy path). |
| tests/NeuralNet/Layers/Swish/SwishTest.php | Adjusts expected float output in Swish backprop test. |
| tests/NeuralNet/FeedForwards/FeedForwardTest.php | Updates imports and assertions to reflect new Networks\FeedForward\FeedForward type hierarchy. |
| src/Serializers/RBX.php | Adds checksum format validation during RBX deserialization. |
| src/Regressors/Ridge.php | Adjusts NumPower usage (transpose signature and aliasing). |
| src/Regressors/MLPRegressor.php | Switches internal network type/docs from Network to FeedForward. |
| src/Regressors/Adaline.php | Switches internal network type/docs from Network to FeedForward. |
| src/NeuralNet/Snapshots/Snapshot.php | Updates snapshot to depend on the new Networks\Base\Contracts\Network. |
| src/NeuralNet/Optimizers/Adam/Adam.php | Adds a local phpdoc hint for $zeros NDArray type. |
| src/NeuralNet/Networks/Network.php | Deletes the old Networks\Network class. |
| src/NeuralNet/Networks/FeedForward/FeedForward.php | Moves/updates FeedForward to implement the new Network contract; adds sample-normalization scaffolding in infer(). |
| src/NeuralNet/Networks/Base/Contracts/Network.php | Introduces a new NDArray-oriented Network contract interface. |
| src/NeuralNet/Network.php | Converts Rubix\ML\NeuralNet\Network into a minimal interface (layers-only). |
| src/NeuralNet/Layers/Multiclass/Multiclass.php | Tightens PHPDoc return type for back() to a shaped array. |
| src/NeuralNet/Layers/Base/Contracts/Parametric.php | Fixes PHPDoc types for parameters/restore signatures. |
| src/NeuralNet/FeedForward.php | Updates legacy FeedForward to implements Network (instead of extending). |
| src/CommitteeMachine.php | Adds an explicit guard when calling type() on an empty expert set. |
| src/Classifiers/SoftmaxClassifier.php | Switches internal network type/docs from Network to FeedForward. |
| src/Classifiers/MultilayerPerceptron.php | Switches internal network type/docs from Network to FeedForward. |
| src/Classifiers/LogisticRegression.php | Switches internal network type/docs from Network to FeedForward. |
| phpstan-baseline.neon | Updates PHPStan baseline (removing some ignores, adding others). |
| CHANGELOG.md | Adds changelog entries for NDArray conversion and Network interface changes. |
Comments suppressed due to low confidence (2)
src/NeuralNet/Networks/FeedForward/FeedForward.php:101
$normalizeSamplesis always initialized tofalseand there is no public API to enable it, so the normalization/reshape branch ininfer()(and thenormalizeSamples()helper) is currently dead code. Either remove the flag+branch, or make normalization configurable (constructor param/setter) and ensure it’s applied consistently in bothinfer()and training paths (roundtrip()), not just inference.
tests/NeuralNet/Networks/NetworkTest.php:26#[CoversClass(Network::class)]now refers to theNetworks\Base\Contracts\Networkinterface (due to the import change), but this test exercisesNetworks\FeedForward\FeedForward. That can make coverage reporting misleading. Consider switching theCoversClasstarget toFeedForward::class(or add an explicit covers attribute for it).
#[Group('NeuralNet')]
#[CoversClass(Network::class)]
class NetworkTest extends TestCase
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use Rubix\ML\Exceptions\InvalidArgumentException; | ||
| use Rubix\ML\Exceptions\RuntimeException; | ||
| use NDArray as nd; | ||
| use NumPower as nd; |
| use Rubix\ML\NeuralNet\Layers\Base\Contracts\Hidden; | ||
| use Rubix\ML\NeuralNet\Layers\Base\Contracts\Input; | ||
| use Rubix\ML\NeuralNet\Layers\Base\Contracts\Output; | ||
| use Rubix\ML\NeuralNet\Layers\Layer; |
Comment on lines
144
to
151
| [$type, $hash] = array_pad(explode(':', $checksum, 2), 2, null); | ||
|
|
||
| if ($type === null || $hash === null) { | ||
| throw new RuntimeException('Invalid checksum format.'); | ||
| } | ||
|
|
||
| if ($hash !== hash($type, $header)) { | ||
| throw new RuntimeException('Header checksum verification failed.'); |
| - RBX Serializer only tracks major library version number | ||
|
|
||
| - Convert NeuralNet classes to use NDArray instead of Matrix | ||
| - Converted back Network interface |
Comment on lines
+10
to
13
| use Rubix\ML\NeuralNet\FeedForward; | ||
| use Rubix\ML\NeuralNet\Layers\Hidden; | ||
| use Rubix\ML\NeuralNet\Layers\Input; | ||
| use Rubix\ML\NeuralNet\Network; |
Author
|
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
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.
No description provided.