Skip to content

/ scikit-learn

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

Jump to bottom

EXA Fix issue in Statistical comparison of models using grid search #19966

Merged
merged 4 commits into from Jun 21, 2021
Merged

EXA Fix issue in Statistical comparison of models using grid search #19966

merged 4 commits into from Jun 21, 2021
+2 −2

Conversation

@olszewskip
Copy link
Contributor

@olszewskip olszewskip commented Apr 23, 2021

Link to the issue: #19949

Modified two lines of code in a single cell in the example
scikit-learn/examples/model_selection/plot_grid_search_stats.py
titled "Statistical comparison of models using grid search".

@martinagvilas
@olszewskip
initial commit
1afb47a
@martinagvilas
martinagvilas reviewed Apr 25, 2021
View changes
examples/model_selection/plot_grid_search_stats.py Outdated Show resolved Hide resolved
@lorentzenchr
Copy link
Member

@lorentzenchr lorentzenchr commented Jun 19, 2021

@olszewskip This seems like a small but nice improvement of the example. If you want to finish it, merging the main branch and fixing linter errors as indicated by @martinagvilas would be the next steps.
olszewskip added 3 commits Jun 20, 2021
@olszewskip
Merge pull request #1 from scikit-learn/main
caaaeee 
update base 20.06.2021
@olszewskip
Merging branch 'main' into issue_#19949 (after updating fork's base).
7cd4422
@olszewskip
In corrected_std func Use kr dummy var Hope to please the linter
7e5b347
@olszewskip
Copy link
Contributor Author

@olszewskip olszewskip commented Jun 20, 2021

Thanks for the reminder. Apologies for the delay.
@olszewskip
Copy link
Contributor Author

@olszewskip olszewskip commented Jun 21, 2021

@lorentzenchr
Sorry, I'm not very experienced with doing pull requests: do I understand correctly that is now up to someone else to review and merge?
@lorentzenchr
Copy link
Member

@lorentzenchr lorentzenchr commented Jun 21, 2021

@olszewskip You‘re correct. I can do it right away. Thanks for updating this PR. CI green, that looks good.
@lorentzenchr
lorentzenchr approved these changes Jun 21, 2021
View changes
Copy link
Member

@lorentzenchr lorentzenchr left a comment

LGTM
@lorentzenchr lorentzenchr changed the title Fix issue "Statistical comparison of models using grid search, is there an error?" DOC Fix issue "Statistical comparison of models using grid search, is there an error?" Jun 21, 2021
@lorentzenchr lorentzenchr changed the title DOC Fix issue "Statistical comparison of models using grid search, is there an error?" EXA Fix issue in Statistical comparison of models using grid search Jun 21, 2021
@lorentzenchr lorentzenchr merged commit c3a3b1e into scikit-learn:main Jun 21, 2021
34 checks passed
34 checks passed
@github-actions
check
Details
@github-actions
check
Details
@github-actions
check
Details
@github-actions
triage
Details
@github-actions
labeler
Details
@github-actions
labeler
Details
@github-actions
Check build trigger
Details
@github-actions
Build wheel for cp${{ matrix.python }}-${{ matrix.platform_id }}-${{ matrix.manylinux_image }}
Details
@github-actions
triage_file_extensions
Details
@github-actions
Source distribution
Details
@github-actions
Upload to Anaconda
Details
@lgtm-com
LGTM analysis: C/C++ No code changes detected
Details
@lgtm-com
LGTM analysis: JavaScript No code changes detected
Details
@lgtm-com
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
@circleci-artifacts-redirector
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
@codecov
codecov/patch Coverage not affected when comparing 617ff6e...7e5b347
Details
@codecov
codecov/project 97.99% (+0.00%) compared to 617ff6e
Details
@azure-pipelines
scikit-learn.scikit-learn Build #20210620.2 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Get Git Commit) Get Git Commit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linting) Linting succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py37_conda_openblas) Linux py37_conda_openblas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux ubuntu_atlas) Linux ubuntu_atlas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux32 debian_atlas_32bit) Linux32 debian_atlas_32bit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux_Runs pylatest_conda_mkl) Linux_Runs pylatest_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Ubuntu_Bionic py37_conda) Ubuntu_Bionic py37_conda succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py37_pip_openblas_32bit) Windows py37_pip_openblas_32bit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_forge_mkl) macOS pylatest_conda_forge_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_mkl_no_openmp) macOS pylatest_conda_mkl_no_openmp succeeded
Details
@lorentzenchr
Copy link
Member

@lorentzenchr lorentzenchr commented Jun 21, 2021

Thanks also to @martinagvilas for confirming that this is better.
@lorentzenchr lorentzenchr mentioned this pull request Jun 21, 2021
Closed
@ogrisel
Copy link
Member

@ogrisel ogrisel commented Jun 22, 2021

What does kr stands for? Maybe a small inline comment could help understanding those lines.
@martinagvilas
Copy link
Contributor

@martinagvilas martinagvilas commented Jun 22, 2021

@ogrisel k stands for number of folds and r stands for repetitions in a repeated k fold cross-validation, so k*r ends up being the number of times the models were evaluated. And I agree this might not be clear from defining kr = len(differences) so an inline comment sounds like a good idea.
@olszewskip olszewskip deleted the olszewskip:issue_#19949 branch Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinagvilas martinagvilas

@lorentzenchr lorentzenchr

Assignees
No one assigned
Labels
Documentation
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@olszewskip @lorentzenchr @ogrisel @martinagvilas