ENH: raise an error when MLP diverges by MarcBresson · Pull Request #29773 · scikit-learn/scikit-learn

MarcBresson · 2024-09-02T15:53:34Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

When MLP weights overflowed with early_stopping=True, the scorer function crashed because it could not compute a validation score. Now, when it overflows, the validation score is replaced by inf, and the model follows its course.

Any other comments?

Because it diverges, early_stopping will not actually stop the model before the number of epochs reached n_max_epochs. One condition for early stopping to trigger is that the new score must be lower or equal to the best score. Since the model diverges, the new score is always greater than the best score (which generally happens on the first epoch)

github-actions · 2024-09-02T15:54:58Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 32c3074. Link to the linter CI: here}

glemaitre · 2024-09-05T15:59:50Z

-            self.validation_scores_.append(self._score(X_val, y_val))
+            try:
+                val_score = self._score(X_val, y_val)
+            except ValueError:


Since we could catch many ValueError, here I assume that we have something in the exception that tell us that this is a np.nan or something like this, isn't it?

good idea, i'll match on that

glemaitre · 2024-09-05T16:00:11Z

+            try:
+                val_score = self._score(X_val, y_val)
+            except ValueError:
+                val_score = np.inf


We will need to add a non-regression test to check that the code works as expected.

glemaitre · 2024-09-05T16:00:38Z

In addition, we will need an entry in the changelog:

Please add an entry to the change log at doc/whats_new/v1.5.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

MarcBresson · 2024-09-07T12:06:53Z

ok, I set the random state to enable reproducible results :) it should be good now, the test passes on my laptop

glemaitre · 2024-09-09T21:05:35Z

I see that we will not be able to accomplished this PR for 1.5.2.
I moved the entry from 1.5.2 to 1.6.

Otherwise everything looks good (I slightly change the entry for the changelog).

glemaitre

Thanks @MarcBresson

Let's have a second review.

glemaitre · 2024-09-09T21:06:21Z

@adrinjalali do you want to have a look after debugging in the PR.

adrinjalali · 2024-09-12T12:55:36Z

+            try:
+                val_score = self._score(X_val, y_val)
+            except ValueError as e:
+                if str(e) == "Input contains NaN.":
+                    val_score = np.inf
+                else:
+                    raise e


This is quite brittle. It depends on the internals and the actual text raised by accuracy_score and r2_score (which are both not ideal lol)

I rather have a check here explicitly for nan, and if we have nan, do the logic, instead of relying on the scorer to raise that exception.

X_val and y_val don't have any NaN. But because model weights are really large numbers, the computation of the score generates NaN values, which, when validation runs, raises the error.

The exception is raised deep in the code (check #29504 for the full traceback) so that's why I ended up just adding the try: except: statement.

I can change the logic behind the _score function to account for that

ok, I did manage to do it better :) thank you for pushing me

adrinjalali · 2024-09-12T12:55:40Z

+    Non-regression test for:
+    https://github.com/scikit-learn/scikit-learn/issues/29504
+    """
+    mlp = MLPRegressor(


We need to test both the regressor and the classifier.

Because a classifier predicts 0s and 1s, the accuracy score will never have NaNs

I'm confused cause the code seems to touch both classifier and regressor, but according to your comment here classifier is fine as is?

adrinjalali · 2024-09-13T08:46:33Z

+:mod:`sklearn.neural_network`
+.............................
+
+- |Fix| :class:`neural_network.MLPClassifier` and :class:`neural_network.MLPRegressor`


So here this doesn't apply to MLPClassifier?

Ups, I'm the culprit here. I think that I change the changelog from the base class to the public classes. So I added MLPClassifier but then it might never have been an issue.

adrinjalali · 2024-09-13T08:47:43Z

-        """Private score method without input validation"""
-        # Input validation would remove feature names, so we disable it
-        return accuracy_score(y, self._predict(X, check_input=False))
+        return super()._score_with_function(X, y, score_function=accuracy_score)


so does this apply to Classifier or not?

it does not apply to classifier, but I did a small refactor so that

# Input validation would remove feature names, so we disable it return accuracy_score(y, self._predict(X, check_input=False))

is only written once :)

adrinjalali · 2024-09-13T08:48:14Z

+    Non-regression test for:
+    https://github.com/scikit-learn/scikit-learn/issues/29504
+    """
+    mlp = MLPRegressor(


I'm confused cause the code seems to touch both classifier and regressor, but according to your comment here classifier is fine as is?

MarcBresson · 2024-09-18T07:55:24Z

@adrinjalali sorry for all the confusion, as much as I love atomic commit, aa7e7a8 was both about implementing your suggestion, and doing a small refactor so that self._predict(X, check_input=False) is only written twice

Co-authored-by: Lock file bot <noreply@github.com>

…29808) Co-authored-by: Lock file bot <noreply@github.com>

…#29809) Co-authored-by: Lock file bot <noreply@github.com>

…29810) Co-authored-by: Lock file bot <noreply@github.com>

Co-authored-by: Mr. Snrub <45150804+s-banach@users.noreply.github.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Shruti Nath <shrutinath@Shrutis-Laptop.local>

…arn#29815)

…istant when the model was strictly diverging with early_stopping

adrinjalali · 2024-09-19T09:12:08Z

@MarcBresson we don't care about commits inside PRs much at all. Better not to force push so that we can see the history. At the end we squash and merge anyway.

For the conflicts, you might want to use the version of the lockfiles in main.

MarcBresson · 2024-09-19T09:17:22Z

noted! The last change was just about changing np.NaN to np.nan :)

…loat("nan") != float("nan")`

… possible and return nan in these cases

MarcBresson force-pushed the main branch from 4035d90 to a502430 Compare September 2, 2024 15:53

github-actions Bot added the module:neural_network label Sep 2, 2024

MarcBresson force-pushed the main branch 2 times, most recently from 51f9830 to 5a37950 Compare September 5, 2024 07:49

glemaitre reviewed Sep 5, 2024

View reviewed changes

MarcBresson force-pushed the main branch 2 times, most recently from 69d14e0 to 4c2bfc3 Compare September 7, 2024 07:14

MarcBresson added 4 commits September 7, 2024 14:06

ENH: avoid MLP from crashing when it diverges.

1095efd

ENH: only match ValueError with a specific message

fbb58d1

TST: add test for diverging MLPs

8d9c86f

TST: set random_state and reduce test size

ddd4701

MarcBresson force-pushed the main branch from 4c2bfc3 to ddd4701 Compare September 7, 2024 12:06

DOC: update changelog for 1.5.2 for fix in scikit-learn#29773

afaf4f9

glemaitre self-requested a review September 9, 2024 20:52

move from 1.5.2 to 1.6

bd81350

glemaitre approved these changes Sep 9, 2024

View reviewed changes

glemaitre added the Waiting for Second Reviewer First reviewer is done, need a second one! label Sep 9, 2024

adrinjalali reviewed Sep 12, 2024

View reviewed changes

adrinjalali reviewed Sep 13, 2024

View reviewed changes

scikit-learn-bot and others added 4 commits September 19, 2024 10:29

🔒 🤖 CI Update lock files for main CI build(s) 🔒 🤖 (scikit-learn#29811)

6c25d10

Co-authored-by: Lock file bot <noreply@github.com>

🔒 🤖 CI Update lock files for scipy-dev CI build(s) 🔒 🤖 (scikit-learn#…

410e203

…29808) Co-authored-by: Lock file bot <noreply@github.com>

🔒 🤖 CI Update lock files for cirrus-arm CI build(s) 🔒 🤖 (scikit-learn…

d364700

…#29809) Co-authored-by: Lock file bot <noreply@github.com>

MAINT: Remove __SKLEARN_SETUP__ from __init__.py (scikit-learn#29803)

2d8c417

scikit-learn-bot and others added 6 commits September 19, 2024 10:29

🔒 🤖 CI Update lock files for array-api CI build(s) 🔒 🤖 (scikit-learn#…

4434782

…29810) Co-authored-by: Lock file bot <noreply@github.com>

Fix sample weight support in ElasticnectCV (scikit-learn#29442)

94876ae

Co-authored-by: Mr. Snrub <45150804+s-banach@users.noreply.github.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Shruti Nath <shrutinath@Shrutis-Laptop.local>

DOC fix entry in changelog for backport happening in 1.5.2 (scikit-le…

237a3f1

…arn#29815)

ENH: check for NaN in prediction results before scoring MLP

73353fc

FIX: initialize _best_coefs and _best_intercepts that were never inex…

23cbd53

…istant when the model was strictly diverging with early_stopping

DOC: update changelog for 1.6 to only mention changes for MLPRegressor

5eefd7f

MarcBresson force-pushed the main branch from bfcdefb to 5eefd7f Compare September 19, 2024 08:29

MarcBresson added 3 commits October 1, 2024 12:17

Merge remote-tracking branch 'upstream/main' into MarcBresson/main

7911daa

TST: fix mlp test by checking the stringified version of nan since `f…

fbfa2b5

…loat("nan") != float("nan")`

FIX: consider that trying to score with infinite prediction should no…

32c3074

… possible and return nan in these cases

adrinjalali approved these changes Oct 14, 2024

View reviewed changes

adrinjalali merged commit ef784c8 into scikit-learn:main Oct 14, 2024

Uh oh!

Conversation

MarcBresson commented Sep 2, 2024

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

github-actions Bot commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Sep 5, 2024

Uh oh!

MarcBresson commented Sep 7, 2024

Uh oh!

glemaitre commented Sep 9, 2024

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Sep 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcBresson Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcBresson commented Sep 18, 2024

Uh oh!

adrinjalali commented Sep 19, 2024

Uh oh!

MarcBresson commented Sep 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions Bot commented Sep 2, 2024 •

edited

Loading

MarcBresson Sep 12, 2024 •

edited

Loading