Ml 385 convert cost functions to num power #387
Conversation
andrewdalpino
left a comment
andrewdalpino
left a comment
There was a problem hiding this comment.
Looks great @apphp! I left a couple comments one on function naming and the other just to clarify our temporary namespacing. Other than that, I think it's good to go. I really liked the additional context you provided by the comments ... I'm generally against comments unless they bring outsized value and you hit it just right with the addition of the mathematical formulas.
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Rubix\ML\NeuralNet\CostFunctions\CrossEntropy; |
There was a problem hiding this comment.
Are you are carving out this namespace to hold the new classes temporarily until we can get the whole Neural Net subsystem converted to NumPower?
There was a problem hiding this comment.
Yes, exactly - like it was done by @SkibidiProduction for activation functions.
src/Traits/ValidatesShapes.php
Outdated
| * @param NDArray $target | ||
| * @throws InvalidArgumentException | ||
| */ | ||
| protected function validateShapes(NDArray $output, NDArray $target) : void |
There was a problem hiding this comment.
To me, this function name does not describe what the function really does. I would recommend something like "validateShapesAreEqual()" or better yet, this should be a method on the NDArray object. Something to consider implementing in NumPower @SkibidiProduction.
There was a problem hiding this comment.
Ok, agree. Renamed into "assertSameShape"
There was a problem hiding this comment.
When we add this method to NDArray object, then we can remove this trait
3 participants
No description provided.