Conversation
# Conflicts: # src/NeuralNet/Networks/FeedForward/FeedForward.php # tests/NeuralNet/Layers/Swish/SwishTest.php # tests/NeuralNet/Networks/NetworkTest.php
There was a problem hiding this comment.
Pull request overview
This PR introduces new namespaced implementations for several regressors (and a couple of dataset generators), updates documentation/examples to point at the new class locations, and expands test coverage via new test suites and shared data providers—along with a few internal utility adjustments to support NumPower usage and stability.
Changes:
- Add new namespaced regressor implementations (e.g.,
Rubix\ML\Regressors\GradientBoost\GradientBoost,...\Ridge\Ridge, etc.) alongside existing top-level counterparts. - Add/extend PHPUnit coverage using new test classes and external data providers.
- Update docs/source links/import examples to reference the new class paths; add small internal helpers (e.g.,
array_pack) and adjust NumPower cloning behavior.
Reviewed changes
Copilot reviewed 69 out of 69 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Regressors/SVR/SVRTest.php | Adds an attribute-based SVR test class under a new folder structure. |
| tests/Regressors/RidgeTest.php | Adds legacy-value regression checks via external data provider. |
| tests/Regressors/Ridge/RidgeTest.php | Adds tests for the new namespaced Ridge implementation (NumPower-based expectations). |
| tests/Regressors/RegressionTreeTest.php | Adds extra trained-model checks using an external provider. |
| tests/Regressors/RegressionTree/RegressionTreeTest.php | New tests for the namespaced RegressionTree implementation. |
| tests/Regressors/RadiusNeighborsRegressorTest.php | Adds prediction-shape/finite-value checks via a local data provider. |
| tests/Regressors/RadiusNeighborsRegressor/RadiusNeighborsRegressorTest.php | New tests for the namespaced RadiusNeighborsRegressor implementation. |
| tests/Regressors/MLPRegressorTest.php | Adds checks for network artifacts (losses/scores) and a training helper. |
| tests/Regressors/MLPRegressors/MLPRegressorTest.php | New, expanded tests for the namespaced MLPRegressor implementation. |
| tests/Regressors/KNNRegressorTest.php | Adds a partial-fit trained-state test via a local provider. |
| tests/Regressors/KNNRegressor/KNNRegressorTest.php | New tests for the namespaced KNNRegressor implementation. |
| tests/Regressors/KDNeighborsRegressor/KDNeighborsRegressorTest.php | New tests for the namespaced KDNeighborsRegressor implementation. |
| tests/Regressors/GradientBoostTest.php | Adds additional training artifacts checks via external provider. |
| tests/Regressors/GradientBoost/GradientBoostTest.php | New tests for the namespaced GradientBoost implementation. |
| tests/Regressors/ExtraTreeRegressorTest.php | Adds provider-driven additional training/prediction checks. |
| tests/Regressors/ExtraTreeRegressor/ExtraTreeRegressorTest.php | New tests for the namespaced ExtraTreeRegressor implementation. |
| tests/Regressors/AdalineTest.php | Adds provider-driven “acceptable values” checks. |
| tests/Regressors/Adaline/AdalineTest.php | New tests for the namespaced Adaline implementation. |
| tests/NeuralNet/NumPower/NumPowerTest.php | Adds NumPower-focused tests (transpose + determinant parity). |
| tests/NeuralNet/Initializers/Xavier/XavierUniformTest.php | Fixes namespace and adjusts constructor test instantiation. |
| tests/NeuralNet/Initializers/Xavier/XavierNormalTest.php | Fixes namespace to match folder/module. |
| tests/NeuralNet/Initializers/Uniform/UniformTest.php | Fixes namespace to match folder/module. |
| tests/NeuralNet/Initializers/Normal/TruncatedNormalTest.php | Fixes namespace to match folder/module. |
| tests/NeuralNet/Initializers/Normal/NormalTest.php | Fixes namespace to match folder/module. |
| tests/NeuralNet/Initializers/LeCun/LeCunUniformTest.php | Adjusts constructor test instantiation. |
| tests/NeuralNet/Initializers/LeCun/LeCunNormalTest.php | Adjusts constructor test instantiation. |
| tests/Datasets/Generators/SwissRoll/SwissRollTest.php | New tests for the namespaced SwissRoll generator. |
| tests/Datasets/Generators/Hyperplane/HyperplaneTest.php | New tests for the namespaced Hyperplane generator. |
| tests/DataProvider/RidgeProvider.php | Adds shared datasets/expected values for Ridge tests (legacy + NumPower variants). |
| tests/DataProvider/RegressionTreeProvider.php | Adds dataset-size cases for RegressionTree tests. |
| tests/DataProvider/GradientBoostProvider.php | Adds dataset-size cases for GradientBoost tests. |
| tests/DataProvider/ExtraTreeRegressorProvider.php | Adds sample datasets for ExtraTreeRegressor provider tests. |
| tests/DataProvider/AdalineProvider.php | Adds sample datasets for Adaline provider tests. |
| tests/CrossValidation/Reports/ErrorAnalysisTest.php | Makes float comparisons tolerant via per-field delta checks. |
| src/Regressors/SVR/SVR.php | Introduces namespaced SVR implementation. |
| src/Regressors/Ridge/Ridge.php | Introduces namespaced Ridge implementation using NumPower algebra path. |
| src/Regressors/Ridge.php | Updates legacy Ridge to use Tensor Matrix/Vector path (drops NumPower usage). |
| src/Regressors/RegressionTree/RegressionTree.php | Introduces namespaced RegressionTree implementation. |
| src/Regressors/RadiusNeighborsRegressor/RadiusNeighborsRegressor.php | Introduces namespaced RadiusNeighborsRegressor implementation. |
| src/Regressors/MLPRegressor/MLPRegressor.php | Introduces namespaced MLPRegressor implementation. |
| src/Regressors/MLPRegressor.php | Minor typing/return cleanup in legacy MLPRegressor. |
| src/Regressors/KNNRegressor/KNNRegressor.php | Introduces namespaced KNNRegressor implementation. |
| src/Regressors/KDNeighborsRegressor/KDNeighborsRegressor.php | Introduces namespaced KDNeighborsRegressor implementation. |
| src/Regressors/GradientBoost/GradientBoost.php | Introduces namespaced GradientBoost implementation. |
| src/Regressors/GradientBoost.php | Minor typing change in legacy GradientBoost. |
| src/Regressors/ExtraTreeRegressor/ExtraTreeRegressor.php | Introduces namespaced ExtraTreeRegressor implementation. |
| src/Regressors/Adaline/Adaline.php | Introduces namespaced Adaline implementation. |
| src/Regressors/Adaline.php | Minor return cleanup in legacy Adaline. |
| src/NeuralNet/Parameters/Parameter.php | Changes cloning strategy to avoid native NDArray clone instability. |
| src/NeuralNet/Networks/FeedForward/FeedForward.php | Switches sample reindexing to array_pack() helper. |
| src/functions.php | Adds array_pack() helper for nested array reindexing. |
| src/Datasets/Generators/SwissRoll/SwissRoll.php | Introduces namespaced SwissRoll generator implementation (NumPower-based). |
| src/Datasets/Generators/Hyperplane/Hyperplane.php | Introduces namespaced Hyperplane generator implementation (NumPower-based). |
| phpstan-ci.neon | Adjusts/extends CI baseline ignores with explicit counts. |
| phpstan-baseline.neon | Updates baseline entries to reflect new warnings introduced by changes. |
| docs/regressors/svr.md | Updates source link and import path for namespaced SVR. |
| docs/regressors/ridge.md | Updates source link and import path for namespaced Ridge. |
| docs/regressors/regression-tree.md | Updates source link and import path for namespaced RegressionTree. |
| docs/regressors/radius-neighbors-regressor.md | Updates source link and import path for namespaced RadiusNeighborsRegressor. |
| docs/regressors/mlp-regressor.md | Updates source link and import paths for namespaced MLPRegressor and NN components. |
| docs/regressors/knn-regressor.md | Updates source link and import path for namespaced KNNRegressor. |
| docs/regressors/kd-neighbors-regressor.md | Updates source link and import path for namespaced KDNeighborsRegressor. |
| docs/regressors/gradient-boost.md | Updates source link and import paths for namespaced GradientBoost/RegressionTree. |
| docs/regressors/extra-tree-regressor.md | Updates source link and import path for namespaced ExtraTreeRegressor. |
| docs/regressors/adaline.md | Updates source link and import paths for namespaced Adaline and NN components. |
| docs/datasets/generators/swiss-roll.md | Updates source link and import path for namespaced SwissRoll generator. |
| docs/datasets/generators/hyperplane.md | Updates source link and import path for namespaced Hyperplane generator. |
| composer.json | Adds rubixml/numpower and apphp/pretty-print dependencies. |
| .github/workflows/ci.yml | Adds a commented-out phpunit invocation hint; keeps composer test as the runner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Prepare samples depending on packing configuration. | ||
| * @param array $samples | ||
| * @return array<int, array<int, mixed>> | ||
| */ | ||
| function array_pack(array $samples) : array | ||
| { | ||
| // Reindex a nested array to ensure all levels have sequential numeric keys | ||
| return array_map('array_values', array_values($samples)); |
There was a problem hiding this comment.
The phpdoc for array_pack() is too generic (@param array $samples) and triggers PHPStan missing iterable value type warnings (see baseline entry). Since this helper is used to normalize nested sample arrays, it should be typed as a list of lists (or array<int, array<int, mixed>>) and the return type should reflect the same to avoid relying on baseline suppressions.
| /** | ||
| * Return dataset sizes for additional RidgeProvider tests with legacy values. | ||
| * | ||
| * @return Generator<string, array{0: int, 1: int}> | ||
| */ |
There was a problem hiding this comment.
This docblock says the provider returns "dataset sizes" and the return type is Generator<string, array{0:int, 1:int}>, but the yielded values are actually (samples, labels, prediction, expectedPrediction, expectedCoefficients, expectedBias). Please update the description and return type to match the yielded structure (same issue also appears on trainPredictProviderForNumPower()).
| <?php | ||
|
|
||
| namespace Rubix\ML\Tests\NeuralNet\NumPower; | ||
|
|
There was a problem hiding this comment.
This test file is missing declare(strict_types=1);, while the rest of the test suite consistently enables strict types. Adding it would keep behavior consistent across the test suite.
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\Attributes\TestDox; | ||
| use PHPUnit\Framework\TestCase; | ||
| use function Apphp\PrettyPrint\pp; | ||
|
|
There was a problem hiding this comment.
use function Apphp\PrettyPrint\pp; is unused in this test file. If it's only here for debugging, it should be removed to avoid carrying an unused dev dependency and import in CI.
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Rubix\ML\Tests\Regressors\SVR; | ||
|
|
||
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\Group; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\Attributes\TestDox; | ||
| use PHPUnit\Framework\TestCase; | ||
| use Rubix\ML\CrossValidation\Metrics\RSquared; | ||
| use Rubix\ML\DataType; | ||
| use Rubix\ML\Datasets\Generators\Hyperplane; | ||
| use Rubix\ML\Datasets\Labeled; | ||
| use Rubix\ML\Datasets\Unlabeled; | ||
| use Rubix\ML\EstimatorType; | ||
| use Rubix\ML\Exceptions\InvalidArgumentException; | ||
| use Rubix\ML\Exceptions\RuntimeException; | ||
| use Rubix\ML\Kernels\SVM\Linear; | ||
| use Rubix\ML\Regressors\SVR; | ||
| use Rubix\ML\Transformers\ZScaleStandardizer; | ||
|
|
||
| #[Group('Regressors')] | ||
| #[CoversClass(SVR::class)] | ||
| class SVRTest extends TestCase |
There was a problem hiding this comment.
There is now a second SVRTest under tests/Regressors/SVR/ that duplicates the existing tests/Regressors/SVRTest.php almost verbatim, which will increase runtime and maintenance burden. Consider consolidating into a single test class or make one of them explicitly target the new namespaced implementation (Rubix\ML\Regressors\SVR\SVR) if both need coverage.
There was a problem hiding this comment.
Not critical, it's a new class for NDArray implementation
composer.json
Outdated
| "swoole/ide-helper": "^5.1", | ||
| "apphp/pretty-print": "^0.6.0" |
There was a problem hiding this comment.
apphp/pretty-print was added as a dev dependency, but the only reference in the PR is an unused import in tests/NeuralNet/NumPower/NumPowerTest.php. If the pretty-printer isn't required for committed tests, please remove the dependency to keep the dev dependency set minimal.
| "swoole/ide-helper": "^5.1", | |
| "apphp/pretty-print": "^0.6.0" | |
| "swoole/ide-helper": "^5.1" |
| } | ||
|
|
||
| $numEstimators = count($this->ensemble); | ||
|
|
There was a problem hiding this comment.
featureImportances() divides by count($this->ensemble) without guarding against an empty ensemble. If training terminates before adding the first booster (e.g., epochs=0, early break on NaN loss, or minChange hit immediately), this becomes a division-by-zero warning and returns INF/NaN importances. Consider throwing when the ensemble is empty or returning a sensible default (e.g., all zeros) before dividing.
| if ($numEstimators === 0) { | |
| return $importances; | |
| } |
…ated annotations and enforced strict types
No description provided.