Skip to content

Improve unit tests #985

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 12 commits into from
Nov 3, 2020
Merged

Improve unit tests #985

mfeurer merged 12 commits into from
Nov 3, 2020

Conversation

@mfeurer
Copy link
Collaborator

@mfeurer mfeurer commented Oct 30, 2020 •
edited
Loading

  • increase wait time when uploading a run to 600s. It's better to wait a very long time instead of failing that test (on CI)
  • reduce the warnings a bit to have less output online
  • remove deprecated format argument to OpenMLDataset
  • add load balancing for parallel unit tests

@mfeurer mfeurer changed the title Randomize test order Improve unit tests Oct 30, 2020
@mfeurer mfeurer force-pushed the randomize_test_order branch from ccec20d to 60bd37a Compare November 2, 2020 09:16
@codecov-io
Copy link

codecov-io commented Nov 2, 2020 •
edited
Loading

Codecov Report

Merging #985 into develop will increase coverage by 0.07%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #985      +/-   ##
===========================================
+ Coverage    87.85%   87.93%   +0.07%     
===========================================
  Files           36       36              
  Lines         4555     4551       -4     
===========================================
  Hits          4002     4002              
+ Misses         553      549       -4
Impacted Files Coverage Δ
openml/flows/flow.py 92.71% <0.00%> (ø)
openml/datasets/dataset.py 83.33% <100.00%> (+0.55%) ⬆️
openml/extensions/sklearn/extension.py 91.08% <100.00%> (ø)
openml/study/functions.py 88.23% <100.00%> (ø)
openml/runs/functions.py 83.58% <0.00%> (+0.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 63ec0ae...39e2f07. Read the comment docs.

PGijsbers
PGijsbers requested changes Nov 2, 2020
View reviewed changes
Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

Overall looks good, just would like some minor clarifications.

tests/test_runs/test_run_functions.py Outdated
n_missing_vals = self.TEST_SERVER_TASK_SIMPLE[1]
n_test_obs = self.TEST_SERVER_TASK_SIMPLE[2]
self._run_and_upload_classification(lr, task_id, n_missing_vals, n_test_obs, "62501")
self.assertLessEqual(warn_mock.call_count, 3)
Copy link
Collaborator

@PGijsbers PGijsbers Nov 2, 2020

Choose a reason for hiding this comment

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

Please explain why we expect three warnings.

Copy link
Collaborator Author

@mfeurer mfeurer Nov 2, 2020

Choose a reason for hiding this comment

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

Honestly, I don't understand the 3 myself and would have expected 1. I will simply increase the max_iter so this test passes.

Copy link
Collaborator Author

@mfeurer mfeurer Nov 2, 2020

Choose a reason for hiding this comment

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

Ah, I now understand. We call run_model_on_task once, and then twice a helper function called _rerun_model_and_compare_predictions. Anyway, I think increasing the number iterations is a bit cleaner.

PGijsbers
PGijsbers requested changes Nov 2, 2020
View reviewed changes
Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

Looks good overall, just expecting some minor explanations.

PGijsbers
PGijsbers reviewed Nov 2, 2020
View reviewed changes
@mfeurer mfeurer requested a review from PGijsbers November 2, 2020 19:03
PGijsbers
PGijsbers approved these changes Nov 2, 2020
View reviewed changes
Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mfeurer mfeurer merged commit a629562 into develop Nov 3, 2020
@mfeurer mfeurer deleted the randomize_test_order branch November 3, 2020 07:35
github-actions bot pushed a commit that referenced this pull request Nov 3, 2020
Matthias Feurer: Improve unit tests (#985)
6612f86
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PGijsbers PGijsbers PGijsbers approved these changes

@sahithyaravi sahithyaravi Awaiting requested review from sahithyaravi

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

@mfeurer @codecov-io @PGijsbers