Convert initializers to NumPower #356
Conversation
SkibidiProduction
requested review from
marclaporte,
mcharytoniuk and
andrewdalpino
SkibidiProduction
changed the title
There was a problem hiding this comment.
Hey @SkibidiProduction overall a pretty good start!
I really like the thoroughness of the unit tests but would encourage you to come up with some more descriptive names for the test methods instead of initializeTest1() initializeTest2() etc. I also like that you broke the initializers into Normal and Uniform versions. Finally, I like that you converted to more modern PHP syntax.
I'm concerned with strict types being unnecessary and slowing down our runtime. I'm also not keen on the idea of introducing additional exception classes without a compelling reason to do so. Also, I think we should stick to calling classes that provide API as "interfaces" and not introduce the "contract" term to mean the same thing as it has the potential to confuse people but provides no additional benefit. Lastly, I think we should avoid creating additional namespace hierarchies unless they are necessary.
| @@ -0,0 +1,38 @@ | |||
| <?php | |||
|
|
|||
| declare(strict_types=1); | |||
There was a problem hiding this comment.
Is this necessary? If not let's not use strict types as it can impact runtime performance.
Enabling strict types can improve code robustness and type safety, but it may also introduce performance overhead due to the additional type-checking at runtime.
There was a problem hiding this comment.
We agreed to leave it at that
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Rubix\ML\NeuralNet\Initializers\Base\Contracts; |
There was a problem hiding this comment.
It would be fine to put this Abstract class directly in the Initializers namespace, without adding a Base or introducing the concept of a "Contract." Rubix ML uses the term "interface" instead of contract and I wouldn't want to confuse people unnecessarily by introducing another name for something that already exists. In addition, this class does not actually provide an interface or contract, it's just a base class.
I think it makes sense to put Exception classes in their own folder, however.
How about we have
There was a problem hiding this comment.
We agreed to leave it temporary
| protected function validateInitParams(int $fanIn, int $fanOut) : void | ||
| { | ||
| if ($fanIn < 1) { | ||
| throw new InvalidFanInException(message: "Fan in cannot be less than 1, $fanIn given"); |
There was a problem hiding this comment.
To me, this exception does not meet the threshold for having it's own class. I would roughly define that threshold as being ...
- Will the user want to catch this exception specifically? - IMO probably not
- Does the exception have complex logic? - It does not
| @@ -0,0 +1,32 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Let's colocate the interface with their implementations and avoid creating unnecessary/duplicate semantics around base classes and interfaces. The PHP language already provides ubiquitous semantics for these concepts.
| * @throws InvalidFanInException Initializer parameter fanIn is less than 1 | ||
| * @throws InvalidFanOutException Initializer parameter fanOut is less than 1 | ||
| */ | ||
| protected function validateInitParams(int $fanIn, int $fanOut) : void |
There was a problem hiding this comment.
This method name is necessarily generic for what it's actually doing. Since this method specifically validates the fan in and fan out perhaps a more accurate name would be validateFanInFanOut().
| { | ||
| $this->validateInitParams(fanIn: $fanIn, fanOut: $fanOut); | ||
|
|
||
| $std = sqrt(2 / ($fanOut + $fanIn)); |
There was a problem hiding this comment.
For consistency, let's use the variable name $stdDev, I believe this is how we call it throughout the codebase.
| @@ -0,0 +1,147 @@ | |||
| <?php | |||
| } | ||
|
|
||
| #[Test] | ||
| #[TestDox('The initializer object os created correctly')] |
There was a problem hiding this comment.
Typo
#[TestDox('The initializer object is created correctly')]| #[Test] | ||
| #[TestDox('The result matrix has correct shape')] | ||
| #[DataProvider('initializeTest1DataProvider')] | ||
| public function initializeTest1(int $fanIn, int $fanOut) : void |
There was a problem hiding this comment.
Can we make these test method names more descriptive?
Example initializeWithCorrectShape()
| #[Test] | ||
| #[TestDox('An exception is thrown during initialization')] | ||
| #[DataProvider('initializeTest3DataProvider')] | ||
| public function initializeTest3(int $fanIn, int $fanOut) : void |
There was a problem hiding this comment.
Can we make these test method names more descriptive?
Example initializeWithBadFanInFanOut()
Initializers have been converted to NumPower.
For He, Xavier, LeCun initializers, a division into two types was made. The first type is based on a truncated normal distribution, the second on a unified distribution.
The final distributions of the initializers are made in accordance with their analogs in the keras library.
A fork of the original NumPower is used (https://github.com/RubixML/numpower) due to the fact that there are difficulties with accepting changes and fixing bugs in the original extension.