Skip to content

396 convert regressors to num power 1#416

Open
apphp wants to merge 16 commits into
RubixML:3.0from
apphp:396-convert-regressors-to-num-power-1
Open

396 convert regressors to num power 1#416
apphp wants to merge 16 commits into
RubixML:3.0from
apphp:396-convert-regressors-to-num-power-1

Conversation

@apphp

@apphp apphp commented Jul 7, 2026

Copy link
Copy Markdown

No description provided.

@apphp apphp requested a review from andrewdalpino July 7, 2026 08:25
@apphp apphp self-assigned this Jul 7, 2026
Comment thread src/Regressors/MLPRegressor.php Outdated
$this->holdOut = $holdOut;
$this->costFn = $costFn ?? new LeastSquares();
$this->metric = $metric ?? new RMSE();
$this->packSamples = $packSamples;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we autodetect whether we need to pack samples or not? If so I'd prefer we did that instead of passing it in at the constructor. What happens if someone passes in packed samples and then unpacked samples after that?

@apphp apphp Jul 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Refactored with auto-detection of arrays, that need pack operation - see array_pack() function

Comment thread src/Regressors/GradientBoost.php Outdated
public function featureImportances() : array
{
if (!isset($this->ensemble, $this->featureCount)) {
if (!$this->ensemble || !$this->featureCount) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This new lines is not the same as the old logic. Why did you change it?

@apphp apphp Jul 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My mistake - turned back.

Comment thread src/Regressors/Adaline.php Outdated
$this->network = new FeedForward(
new Placeholder1D($dataset->numFeatures()),
[new Dense(1, $this->l2Penalty, true, new Xavier2())],
[new Dense(1, $this->l2Penalty, true, new XavierUniform())],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since XavierUniform implements Xavier1 initialization, this change is not equivalent to the old line. We'll need to implement Xavier2Uniform and rename XavierUniform to Xavier1Uniform.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed with Xavier2Uniform

Comment thread src/Classifiers/SoftmaxClassifier.php Outdated
$this->network = new FeedForward(
new Placeholder1D($dataset->numFeatures()),
[new Dense(count($classes), $this->l2Penalty, true, new Xavier1())],
[new Dense(count($classes), $this->l2Penalty, true, new XavierNormal())],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

XavierNormal is not equivalent to Xavier1 which samples from a uniform distribution.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed with Xavier1Uniform

$hiddenLayers = $this->hiddenLayers;

$hiddenLayers[] = new Dense(count($classes), 0.0, true, new Xavier1());
$hiddenLayers[] = new Dense(count($classes), 0.0, true, new XavierNormal());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

XavierNormal is not equivalent to Xavier1 which samples from a uniform distribution.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed with using Xavier1Uniform

Comment thread src/Classifiers/LogisticRegression.php Outdated
$this->network = new FeedForward(
new Placeholder1D($dataset->numFeatures()),
[new Dense(1, $this->l2Penalty, true, new Xavier1())],
[new Dense(1, $this->l2Penalty, true, new XavierNormal())],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

XavierNormal is not equivalent to Xavier1 which samples from a uniform distribution.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed with using Xavier1Uniform

Comment thread src/Regressors/GradientBoost.php Outdated
Comment thread phpstan-bootstrap.php
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