Skip to content

[MRG+1] EHN/DOC Make error msg more informative in Pipeline #8865

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.

Sign up for GitHub

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
Merged
merged 5 commits into from
May 17, 2017
Merged

[MRG+1] EHN/DOC Make error msg more informative in Pipeline #8865

merged 5 commits into from
May 17, 2017

Conversation

glemaitre
Copy link
Member

Reference Issue

None

What does this implement/fix? Explain your changes.

The message in Pipeline can be tricky to understand. If a joblib.Memory is passed instead of
sklearn.externals.joblib.Memory, the error message mention that we should provide a joblib.Memory, we got memory=Memory(...) instead.

This PR intends to:

  • Make the error more explicit by printing the type of the object passed in the error message;
  • Write explicitly in the documentation that Memory has to be from sklearn.externals.joblib

Any other comments?

Another solution is to make some ducktyping: we can remove the isinstance by checking that the object has a known joblib.Memory method. However, it might open the door to some bugs due to different versions of internal and external joblib.

@glemaitre
Copy link
Member Author

The artifact showing that the numpydoc is fine.

@jnothman @lesteve any thoughts?

@lesteve
Copy link
Member

lesteve commented May 12, 2017 •
edited
Loading

I seem to remember there were other instances of joblib.Memory in docstrings and/or error messages in other places in the code. Maybe we should fix them in the same PR ?

An alternative is to check for a cache attribute that is a method. This is probably good enough for caching purposes.

Not sure whether one solution is strongly preferable over the other.

@glemaitre
Copy link
Member Author

Maybe we should fix them in the same PR ?

Agreed, I'll check that.

@TomDLT TomDLT changed the title [MRG] EHN/DOC Make error msg more informative in Pipeline [MRG+1] EHN/DOC Make error msg more informative in Pipeline May 16, 2017
@codecov
Copy link

codecov bot commented May 16, 2017 •
edited
Loading

Codecov Report

Merging #8865 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8865   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files         333      333           
  Lines       61161    61161           
=======================================
  Hits        58425    58425           
  Misses       2736     2736
Impacted Files Coverage Δ
sklearn/tests/test_pipeline.py 99.62% <ø> (ø) ⬆️
sklearn/pipeline.py 99.57% <ø> (ø) ⬆️
sklearn/feature_extraction/text.py 96.05% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 719afba...5cb811c. Read the comment docs.

@glemaitre
Copy link
Member Author

I fixed the comment of @lesteve. Also, I memory has a mutable argument in randomized_l1 which I corrected using None as default like for the other cases.

jnothman
jnothman approved these changes May 17, 2017
View reviewed changes
Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

LGTM if you're finished.

@glemaitre
Copy link
Member Author

@jnothman @TomDLT Done and all green.

@jnothman jnothman merged commit 4c33ea2 into scikit-learn:master May 17, 2017
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
@glemaitre @Sundrique
[MRG+1] EHN/DOC Make error msg about Memory more explicit (scikit-lea…
eea2d07 
…rn#8865)
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@glemaitre @dmohns
[MRG+1] EHN/DOC Make error msg about Memory more explicit (scikit-lea…
5e6fdab 
…rn#8865)
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@glemaitre @dmohns
[MRG+1] EHN/DOC Make error msg about Memory more explicit (scikit-lea…
06e4347 
…rn#8865)
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
@glemaitre @NelleV
[MRG+1] EHN/DOC Make error msg about Memory more explicit (scikit-lea…
881be97 
…rn#8865)
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@glemaitre @paulha
[MRG+1] EHN/DOC Make error msg about Memory more explicit (scikit-lea…
a31e0e3 
…rn#8865)
AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
@glemaitre @AishwaryaRK
[MRG+1] EHN/DOC Make error msg about Memory more explicit (scikit-lea…
40bbffe 
…rn#8865)
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@glemaitre @maskani-moh
[MRG+1] EHN/DOC Make error msg about Memory more explicit (scikit-lea…
4f9911a 
…rn#8865)
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@glemaitre
[MRG+1] EHN/DOC Make error msg about Memory more explicit (scikit-lea…
8359834 
…rn#8865)
@lesteve lesteve mentioned this pull request Jan 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman approved these changes

@TomDLT TomDLT TomDLT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@glemaitre @lesteve @jnothman @TomDLT