Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] 32-bit support for MLP #17759

Merged
merged 13 commits into from Jul 4, 2020
Merged

[MRG] 32-bit support for MLP #17759

merged 13 commits into from Jul 4, 2020

Conversation

postmalloc
Copy link
Contributor

Reference Issues/PRs

Fixes #17700

What does this implement/fix? Explain your changes.

Includes float32 support for MLPClassifier and MLPRegressor.

@postmalloc postmalloc changed the title [MRG] 32-bit support for MLP [WIP] 32-bit support for MLP Jun 27, 2020
@postmalloc postmalloc changed the title [WIP] 32-bit support for MLP [MRG] 32-bit support for MLP Jun 27, 2020
@harishB97
Copy link

Hi @d3b0unce,
May I know whether the call for check_array in line 328 is necessary. When we call validate_input following that in line 333, it calls the check_array and does the type casting once again. Can we pass the dtype tuple directly to _validate_data in line 977.

@postmalloc
Copy link
Contributor Author

@harishB97, thank you for pointing it out. That call was indeed redundant. Passing the tuple directly to _validate_data now.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @d3b0unce ! I think there is 1 occurrences missing,

Also could you please add a check to test that coef, intercept (and predictions only in the case of MLPRegressor) are of dtype float32 when X is float32?

multi_output=True)
multi_output=True,
dtype=(np.float64, np.float32))
self._dtype = X.dtype
Copy link
Member

@rth rth Jun 29, 2020

Choose a reason for hiding this comment

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

So right now _validate_input is indeed only called on fit, but generally I think it would be safer to make it not store anything on self in case someone uses it for predict in the future. Using X.dtype later should be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I had assigned it to self as I needed dtype in _init_coef where X is not in scope. Now I am passing dtype as a parameter to _init_coef.

@postmalloc
Copy link
Contributor Author

Thank you @rth, for the suggestions! I added tests to check dtypes of parameters.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @d3b0unce !

@@ -985,7 +993,7 @@ def _validate_input(self, X, y, incremental):
" `self.classes_` has %s. 'y' has %s." %
(self.classes_, classes))

y = self._label_binarizer.transform(y)
y = self._label_binarizer.transform(y).astype(np.bool)
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason behind this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this because the call to _label_binarizer.transform returns int64, which was upcasting other network parameters to float64 during _backprop.

Copy link
Member

Choose a reason for hiding this comment

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

Let's include this explanation as a comment in the code. (This downcast to bool is to prevent upcasting when working with float32 data)

assert_allclose(pred_64, pred_32, rtol=1e-04)


def test_mlp_param_dtypes():
Copy link
Member

Choose a reason for hiding this comment

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

This can be parametrize:

@pytest.mark.parametrize('dtype', [np.float32, np.float64])
def test_mlp_param_dtypes(dtype):
	# Checks input dtype is used for attributes and prediction

	X, y = X_digits[:300].astype(dtype), y_digits[:300]
    mlp = MLPRegressor(alpha=1e-5,
                          hidden_layer_sizes=(5, 3),
                          random_state=1, max_iter=50)
    mlp.fit(X, y)
    pred = mlp_64.predict(X)

	assert pred.dtype == dtype
	...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

hidden_layer_sizes=(5, 3),
random_state=1, max_iter=100)
mlp_64.fit(X_digits[:300], y_digits[:300])
pred_64 = mlp_64.predict(X_digits[300:])
Copy link
Member

Choose a reason for hiding this comment

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

We should check for predict_proba as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, added a test

@postmalloc
Copy link
Contributor Author

Thank you @thomasjpfan for the suggestions! I've committed the changes.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

One last comment otherwise LGTM, thanks:

# Checks if input dtype is used for network parameters
# and predictions
X, y = X_digits.astype(dtype), y_digits
mlp = MLPRegressor(alpha=1e-5,
Copy link
Member

Choose a reason for hiding this comment

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

Could you also parametrize this test function with Estimator in [MLPClassifier, MLPRegressor] . Fitting on y should work in both cases since it is int. And then the last assert only do it in the case of MLPRegressor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@rth
Copy link
Member

rth commented Jul 1, 2020

Also please add an entry to the change log at doc/whats_new/v0.24.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself with :user:.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @d3b0unce !

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

One small comment, otherwise LGTM!

Thank you @d3b0unce !

@@ -985,7 +993,7 @@ def _validate_input(self, X, y, incremental):
" `self.classes_` has %s. 'y' has %s." %
(self.classes_, classes))

y = self._label_binarizer.transform(y)
y = self._label_binarizer.transform(y).astype(np.bool)
Copy link
Member

Choose a reason for hiding this comment

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

Let's include this explanation as a comment in the code. (This downcast to bool is to prevent upcasting when working with float32 data)

@postmalloc
Copy link
Contributor Author

Thanks @thomasjpfan! Added the comment.

@rth rth merged commit 9fc0006 into scikit-learn:master Jul 4, 2020
7 checks passed
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Jul 17, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add 32 bit support to neural_network module
4 participants