Skip to content

FIX Fix properly convert cv_results_ values to numpy array #19211

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

Merged
merged 7 commits into from
Apr 14, 2021

Conversation

NicolasHug
Copy link
Member

Fixes #19203

Some values in the cv_results_ attribute of the SH estimators where lists instead of numpy arrays.

CC @glemaitre as this might be release-critical?

@glemaitre
Copy link
Member

CC @glemaitre as this might be release-critical?

We discuss with @ogrisel and it is not critical for 0.24.1 but we will include it in the upcoming 0.24.2.

@glemaitre glemaitre added this to the 0.24.2 milestone Jan 19, 2021
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.

In general this looks like a common test for *SearchCV.cv_results_, but we can do that in a future PR.

@jnothman
Copy link
Member

jnothman commented Jan 19, 2021 via email

Base automatically changed from master to main January 22, 2021 10:53
@thomasjpfan thomasjpfan changed the title [MRG] Fix properly convert cv_results_ values to numpy array FIX Fix properly convert cv_results_ values to numpy array Apr 13, 2021
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.

Move the whats new to 0.24.2. LGTM on my end.

@glemaitre easy review?

for key, val in results.items():
# each value is a list (as per evaluate_candidate's convention)
# we convert it to an array for consistency with the other keys
results[key] = np.asarray(val)
Copy link
Member

Choose a reason for hiding this comment

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

I updated this to use asarray so we do not make a copy if val is already an ndarray.

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM

@glemaitre glemaitre merged commit 872052b into scikit-learn:main Apr 14, 2021
thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request Apr 19, 2021
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 22, 2021
glemaitre pushed a commit that referenced this pull request Apr 28, 2021
…19211)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
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.

SuccessiveHalving's cv_results_['iter'] is a list unlike other cv_results_ values
5 participants