Skip to content

FIX SGDRegreesor and SGDClassifier use correct number of validation data#23256

Merged
jeremiedbb merged 8 commits intoscikit-learn:mainfrom
MaxwellLZH:fix/sgd-validation-mask
May 2, 2022
Merged

FIX SGDRegreesor and SGDClassifier use correct number of validation data#23256
jeremiedbb merged 8 commits intoscikit-learn:mainfrom
MaxwellLZH:fix/sgd-validation-mask

Conversation

@MaxwellLZH
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Fixes #23255.

What does this implement/fix? Explain your changes.

  • changes the validation_mask to be a boolean array for correct indexing.
  • fixes a failing test case.

Any other comments?

I haven't added any test cases because I'm not quite sure how to test this behavior without directly using internal api like checking against _ValidationScoreCallback.X_val.shape.

Copy link
Copy Markdown
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!

I think this is a big enough bug to place into 1.1. The behavior on main only uses rows 0 and 1 for validation.

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

@thomasjpfan thomasjpfan added this to the 1.1 milestone May 1, 2022
@thomasjpfan thomasjpfan added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label May 1, 2022
MaxwellLZH and others added 3 commits May 1, 2022 22:45
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Copy link
Copy Markdown
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.

Minor comments, otherwise LGTM

MaxwellLZH and others added 3 commits May 1, 2022 23:07
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @MaxwellLZH !

@jeremiedbb jeremiedbb merged commit 6231e1e into scikit-learn:main May 2, 2022
jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this pull request May 10, 2022
…ata (scikit-learn#23256)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module:linear_model To backport PR merged in master that need a backport to a release branch defined based on the milestone.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG SGDRegressor only using the first two samples for validation error calculation

3 participants