396 convert regressors to num power#411
Conversation
# Conflicts: # src/NeuralNet/Networks/FeedForward/FeedForward.php # tests/NeuralNet/Layers/Swish/SwishTest.php # tests/NeuralNet/Networks/NetworkTest.php
andrewdalpino
left a comment
andrewdalpino
left a comment
There was a problem hiding this comment.
Left some comment @apphp, will continue the review sometime in the future.
| run: composer analyze-ci | ||
|
|
||
| - name: Unit Tests | ||
| #run: vendor/bin/phpunit --display-warning --display-deprecations --display-notices --testsuite="Anomaly Detectors,Backends,Base,Classifiers,Clusterers,Cross Validation,Datasets,Extractors,Graph,Helpers,Kernels,Loggers,NeuralNet,Persisters,Regressors,Serializers,Specifications,Strategies,Tokenizers,Transformers" |
There was a problem hiding this comment.
What is the purpose of this line?
There was a problem hiding this comment.
Just possibility to run tests separately. Sometimes they failed on GitHub, so to save time I can run only problematic section. If you don't like it - I can remove this line.
| @@ -1,4 +1,4 @@ | |||
| <span style="float:right;"><a href="https://github.com/RubixML/ML/blob/master/src/Datasets/Generators/Hyperplane.php">[source]</a></span> | |||
| <span style="float:right;"><a href="https://github.com/RubixML/ML/blob/master/src/Datasets/Generators/Hyperplane/Hyperplane.php">[source]</a></span> | |||
There was a problem hiding this comment.
This will go back to https://github.com/RubixML/ML/blob/master/src/Datasets/Generators/Hyperplane.php before release correct?
There was a problem hiding this comment.
Why? This is a new class for migration to NumPower. Currently we store old files (Martix) and new (NumPower) approach. If you decide to remove old files at all before release, so yes - this will go back before final release of v3.0
| { | ||
| $samples = $labels = []; | ||
|
|
||
| $counts = NumPower::round(NumPower::multiply($this->weights, $n), 0)->toArray(); |
There was a problem hiding this comment.
Why are we using numpower instead of PHP here? Is it justified?
There was a problem hiding this comment.
No, you're right. I used $this->weights as NDArray, that's why I was needed to changed this method.
But I've redefined $this->weights as array (like in original class) and now generate() methods looks exactly like in previous class.
| * | ||
| * @var NDArray | ||
| */ | ||
| protected NDArray $weights; |
There was a problem hiding this comment.
Does this need to be a NumPower array?
| } | ||
| } | ||
|
|
||
| if (is_array($weights)) { |
There was a problem hiding this comment.
Let's stick to either a PHP array or a NumPower array in the API, but not both.
There was a problem hiding this comment.
Fixed back to original version - array
|
|
||
| $weights = NumPower::divide($weights, $total); | ||
| } else { | ||
| $weights = NumPower::array(array_fill(0, $k, 1.0 / $k)); |
There was a problem hiding this comment.
Do we not need any of the bounds checking for NumPower like we do above with a PHP array?
There was a problem hiding this comment.
I just tried to stay on original file code:
foreach ($weights as &$weight) {
$weight /= $total;
}
} else {
$weights = array_fill(0, $k, 1.0 / $k);
}
|
|
||
| $samples = NumPower::add( | ||
| NumPower::multiply( | ||
| NumPower::normal(size: [$n, $d], loc: 0.0, scale: 1.0), |
There was a problem hiding this comment.
What does the loc argument do here? Is it some type of offset?
There was a problem hiding this comment.
This is just a default parameter with default values - removed.
@param int|float|string $loc Mean (µ); default 0.0.
| @@ -0,0 +1,116 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Have you tried plotting the output of these Generators as a sanity check?
There was a problem hiding this comment.
No. Can you explain me how to do that?
… NumPower::normal
… NumPower::normal
No description provided.