Skip to content

ML-400 Converted back Network interface#403

Open
apphp wants to merge 4 commits intoRubixML:3.0from
apphp:400-refactor-network-class
Open

ML-400 Converted back Network interface#403
apphp wants to merge 4 commits intoRubixML:3.0from
apphp:400-refactor-network-class

Conversation

@apphp
Copy link

@apphp apphp commented Mar 11, 2026

No description provided.

@apphp apphp requested a review from andrewdalpino March 11, 2026 00:23
@apphp apphp self-assigned this Mar 11, 2026
@apphp
Copy link
Author

apphp commented Mar 11, 2026

All tests were passed

image

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reintroduces a neural-network “Network” interface and updates instantiation sites/tests to construct FeedForward networks instead of the removed Network concrete class, alongside reorganizing the NDArray-based network code under NeuralNet\Networks\Base\....

Changes:

  • Replace new Network(...) with new FeedForward(...) across learners and tests.
  • Introduce src/NeuralNet/Networks/Base/Contracts/Network.php and refactor the NDArray-based FeedForward network to implement it; remove the old src/NeuralNet/Networks/Network.php.
  • Update snapshot/network tests and adjust one Swish layer expected value; update changelog entries.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/NeuralNet/Snapshots/SnapshotTest.php Switch snapshot test to NDArray “Base” FeedForward + new Network contract.
tests/NeuralNet/SnapshotTest.php Switch legacy snapshot test to instantiate FeedForward instead of Network.
tests/NeuralNet/Networks/NetworkTest.php Update NDArray network test imports and construction to use Base FeedForward.
tests/NeuralNet/NetworkTest.php Switch legacy network test construction to FeedForward.
tests/NeuralNet/Layers/Swish/SwishTest.php Update expected numeric output to match current computation.
tests/NeuralNet/FeedForwards/FeedForwardTest.php Point feedforward tests at Base FeedForward and new contracts.
src/Regressors/MLPRegressor.php Use FeedForward for the underlying network construction.
src/Regressors/Adaline.php Use FeedForward for the underlying network construction.
src/NeuralNet/Snapshots/Snapshot.php Update snapshot to accept Base Network contract.
src/NeuralNet/Networks/Network.php Remove the old NDArray Network concrete class.
src/NeuralNet/Networks/Base/FeedForward/FeedForward.php Move/rename Base FeedForward, implement Base Network contract, add sample-normalization branch.
src/NeuralNet/Networks/Base/Contracts/Network.php Add Base Network interface (input/hidden/output/layers).
src/NeuralNet/Network.php Convert legacy Network to an interface (currently only layers()).
src/NeuralNet/FeedForward.php Update legacy FeedForward to implement the legacy Network interface.
src/Classifiers/SoftmaxClassifier.php Construct FeedForward network instead of Network.
src/Classifiers/MultilayerPerceptron.php Construct FeedForward network instead of Network.
src/Classifiers/LogisticRegression.php Construct FeedForward network instead of Network.
CHANGELOG.md Note NDArray conversion + “converted back Network interface” entry.
Comments suppressed due to low confidence (2)

src/NeuralNet/Networks/Base/FeedForward/FeedForward.php:214

  • The empty-dataset fast path (if ($dataset->empty()) return ...) and the 1D-shape reshape are currently guarded by $this->normalizeSamples, but that flag is never enabled. If these safeguards are needed to avoid NumPower edge cases, they should be applied unconditionally (or the flag needs to be configurable) so inference on empty/non-packed samples is actually protected.
    src/NeuralNet/Networks/Base/FeedForward/FeedForward.php:101
  • $this->normalizeSamples is initialized to false and the property is private, but there is no constructor parameter or method to ever set it to true (confirmed by repo-wide search). This makes the normalization/reshape logic in infer() and the normalizeSamples() helper effectively dead code. Consider either removing the flag and always normalizing (if required for NumPower), or exposing a supported way to enable it (constructor arg or public setter).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants