8000 MNT Use raise from in 19 modules by cool-RR · Pull Request #17835 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MNT Use raise from in 19 modules #17835

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 1 commit into from
Aug 25, 2020
Merged

MNT Use raise from in 19 modules #17835

merged 1 commit into from
Aug 25, 2020

Conversation

cool-RR
Copy link
Contributor
@cool-RR cool-RR commented Jul 5, 2020

@NicolasHug This is the continuation of #17545

After we're done with this PR, I'll do another PR to fix the rest.

@cool-RR cool-RR force-pushed the 2020-07-05-raise-from branch 3 times, most recently from 8f56e68 to ad29136 Compare July 5, 2020 09:00
NicolasHug
NicolasHug reviewed Jul 6, 2020
View reviewed changes
Copy link
Member
@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

thanks @cool-RR , a few comments

@cool-RR cool-RR force-pushed the 2020-07-05-raise-from branch 4 times, most recently from a5de13e to df00a89 Compare July 6, 2020 12:41
@cmarmo
Copy link
Contributor
cmarmo c 8000 ommented Jul 9, 2020

Hi @cool-RR codecov/patch check is unhappy: clicking on the Details link will give you all the information.

For example, for one of the files sklearn/inspection/_partial_dependence.py you can check here the lines of the code that are not tested in red.
A specific test should be added in those cases.
Thanks!

@cool-RR
Copy link
Contributor Author
cool-RR commented Jul 9, 2020

@cmarmo Ah... We didn't do any of that testing in the previous PRs in this series. I'm a little reluctant to dig into scikit-learn's test architecture, because I went through a lot of files making the same change. Any chance I could get a pass on this?

@cmarmo
Copy link
Contributor
cmarmo commented Jul 9, 2020

I haven't check in details where the codecov report has failed. A nice summary tells you which files are affected in your pull request.
I didn't know how to write tests in scikit-learn until last year too, don't let you intimidate... :)
You can check the documentation, but as an example for _partial_dependence.py you should add a test in sklearn/inspection/tests/test_partial_dependence.py.

  • First you create a simple code reproducing the error that should be tested
  • then you verify that it is thrown using pytest, like here

Additional resources about tests are also available here.

Any chance I could get a pass on this?

I'm not the one deciding, sorry .. :).

@cool-RR
Copy link
Contributor Author
cool-RR commented Aug 11, 2020

@NicolasHug Because I haven't gotten a reply on this PR for a long time, it now has a conflict. I can solve the merge conflict, but I first want to know whether you are even interested in accepted this PR, and whether there are any problems besides the merge conflict that would prevent this PR from being merged.

@thomasjpfan
Copy link
Member

I would be +1 for merging this.

@cool-RR cool-RR force-pushed the 2020-07-05-raise-from branch from df00a89 to 13c0e6e Compare August 14, 2020 14:40
@cool-RR
Copy link
Contributor Author
cool-RR commented Aug 14, 2020

@thomasjpfan I'm seeing the CI is failing with this line: INTERNALERROR> AttributeError: 'WorkerController' object has no attribute 'slaveinput'

This doesn't seem related to my changes. Any idea why it's failing? How can I fix this?

@thomasjpfan
Copy link
Member

This should have been fixed with pytest-dev/pytest-xdist#287

It could be an issue with pytest 6.0 I'll look into it.

@cool-RR
Copy link
Contributor Author
cool-RR commented Aug 14, 2020 via email

@thomasjpfan
Copy link
Member
thomasjpfan commented Aug 14, 2020 •
edited
Loading

The issue will resolve with pytest-dev/pytest-cov#411 when pytest-cov==2.10.1 gets released. It was caused by pytest-xdist updating to 2.0.

There is no action we can take in this PR for now.

I have a temporary fix at #17835

@cool-RR cool-RR force-pushed the 2020-07-05-raise-from branch from 13c0e6e to ca06555 Compare August 18, 2020 12:05
@cool-RR
Copy link
Contributor Author
cool-RR commented Aug 18, 2020

@thomasjpfan I assume that the codecov/patch is something I can ignore, right? If so, are we good for merging this PR?

thomasjpfan
thomasjpfan reviewed Aug 18, 2020
View reviewed changes
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.

As for codecov, this means that some of these exceptions are not tested. I think I am okay with ignoring them in this PR.

@cool-RR cool-RR force-pushed the 2020-07-05-raise-from branch from ca06555 to 9d82cb8 Compare August 19, 2020 06:05
@cool-RR
Copy link
Contributor Author
cool-RR commented Aug 19, 2020

@thomasjpfan Anything else needed before merging?

rth
rth approved these changes Aug 25, 2020
View reviewed changes
Copy link
Member
@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @cool-RR !

@rth rth merged commit 61ec6b9 into scikit-learn:master Aug 25, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@cool-RR @jayzed82
MNT Use raise from in 19 modules (scikit-learn#17835)
c13cb2f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasHug NicolasHug NicolasHug left review comments

@rth rth rth approved these changes

@thomasjpfan thomasjpfan thomasjpfan 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.
5 participants
@cool-RR @cmarmo @thomasjpfan @rth @NicolasHug
0