Skip to content

Optimizing Perrone example #853

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
mfeurer merged 4 commits into from
Oct 23, 2019
Merged

Optimizing Perrone example #853

mfeurer merged 4 commits into from
Oct 23, 2019

Conversation

@Neeratyoy
Copy link
Contributor

@Neeratyoy Neeratyoy commented Oct 19, 2019

Reference Issue

Addresses #852.

What does this PR implement/fix? Explain your changes.

Removes the repeated calls to openml.runs.get_runs and replaces it with one single API call using openml.evaluations.list_evaluations_setups.
Also, added a unit test for this example.

@codecov-io
Copy link

codecov-io commented Oct 19, 2019 •
edited
Loading

Codecov Report

Merging #853 into develop will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop    #853     +/-   ##
=========================================
- Coverage    88.49%   88.4%   -0.1%     
=========================================
  Files           37      37             
  Lines         4269    4269             
=========================================
- Hits          3778    3774      -4     
- Misses         491     495      +4
Impacted Files Coverage Δ
openml/datasets/functions.py 94.06% <0%> (-1.25%) ⬇️

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 f74b73a...dc1ce33. Read the comment docs.

@Neeratyoy Neeratyoy marked this pull request as ready for review October 19, 2019 23:18
mfeurer
mfeurer reviewed Oct 22, 2019
View reviewed changes
Copy link
Collaborator

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this looks really good. However, could you please explain why you need an extra test given that the example is executed by travis-ci?

@mfeurer mfeurer mentioned this pull request Oct 22, 2019
Merged
@Neeratyoy
Copy link
Contributor Author

Neeratyoy commented Oct 22, 2019

However, could you please explain why you need an extra test given that the example is executed by travis-ci?

I came across the issue #805 and thought this was a good opportunity to incorporate it. Also, I came across a unit test for the SVM example. Hence, I thought this was the protocol.
Having the example as a unit test also seemed to be a good check since from what I understand, we won't know if the example script failed, in terms of an error from travis-ci, will we? That is, we might still get all greens.

@mfeurer mfeurer merged commit 433f1e7 into develop Oct 23, 2019
@mfeurer mfeurer deleted the fix_852 branch October 23, 2019 12:40
@mfeurer mfeurer mentioned this pull request Oct 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mfeurer mfeurer mfeurer approved these changes

@PGijsbers PGijsbers Awaiting requested review from PGijsbers

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

@Neeratyoy @codecov-io @mfeurer