Skip to content
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

Add unittests for updaters. #3836

Merged
merged 1 commit into from Nov 7, 2018
Merged

Add unittests for updaters. #3836

merged 1 commit into from Nov 7, 2018

Conversation

trivialfis
Copy link
Member

This is to aid #3825. Without unittests it's very hard for me to do refactoring since I can't interpret errors in r-tests.

@trivialfis
Copy link
Member Author

Note to myself, parameter int colmat_dtype is removed.

@codecov-io
Copy link

codecov-io commented Oct 28, 2018 •
edited

Codecov Report

Merging #3836 into master will increase coverage by 4.42%.
The diff coverage is 51.97%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3836      +/-   ##
============================================
+ Coverage     52.05%   56.48%   +4.42%     
  Complexity      203      203              
============================================
  Files           181      184       +3     
  Lines         14360    14502     +142     
  Branches        495      495              
============================================
+ Hits           7475     8191     +716     
+ Misses         6647     6073     -574     
  Partials        238      238
Impacted Files Coverage Δ Complexity Δ
src/metric/elementwise_metric.cc 69.46% <ø> (ø) 0 <0> (ø) ⬇️
src/common/io.h 71.42% <ø> (ø) 0 <0> (ø) ⬇️
src/common/hist_util.cc 42.72% <ø> (+9.09%) 0 <0> (ø) ⬇️
src/tree/updater_colmaker.cc 1.59% <ø> (ø) 0 <0> (ø) ⬇️
tests/cpp/tree/test_param.cc 100% <ø> (ø) 0 <0> (ø) ⬇️
src/metric/rank_metric.cc 86.01% <ø> (ø) 0 <0> (ø) ⬇️
src/metric/multiclass_metric.cc 88.67% <ø> (ø) 0 <0> (ø) ⬇️
src/tree/updater_basemaker-inl.h 0% <ø> (ø) 0 <0> (ø) ⬇️
src/tree/updater_prune.cc 90.9% <ø> (+79.54%) 0 <0> (ø) ⬇️
src/tree/updater_sync.cc 47.61% <ø> (+23.8%) 0 <0> (ø) ⬇️
... and 31 more

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 d81fedb...01c388b. Read the comment docs.

@hcho3
Copy link
Collaborator

hcho3 commented Oct 28, 2018

@trivialfis This is great! Let me know if you want me to review.

@trivialfis trivialfis changed the title [WIP] Add unittests for updaters. Add unittests for updaters. Oct 29, 2018
@trivialfis
Copy link
Member Author

trivialfis commented Oct 29, 2018 •
edited

@hcho3 I'm afraid there is not much I can do. It's very hard for me to come up with proper tests for these algorithms now.
For simple one like prune, it's still possible to invent some data for specific testing cases by reading the source code, but for more complicate algorithms like fast_hist, it's best to come up with these test cases before or during development.
What I'm doing for fast_hist is just assuming it's correct, then create a simple test to "lock" the current output of it so that future changes won't be able to break it.

@trivialfis trivialfis requested a review from hcho3 October 29, 2018 04:16
@trivialfis trivialfis mentioned this pull request Oct 29, 2018
Merged
@hcho3
Copy link
Collaborator

hcho3 commented Oct 29, 2018

@trivialfis I can add some tests for fast_hist after 0.81 release. Can we hold a bit?

@trivialfis
Copy link
Member Author

trivialfis commented Oct 29, 2018 •
edited

@hcho3 That would be great. In the mean while can you help taking a look into these thing in this PR?

  • Renamed fast_hist file into quantile_hist, spited the interface and impl, removed fast_hist parameter.
  • The current method of adding unittest for fast_hist. (By inheritance).

If you find these unsuitable, I can make changes early.

@trivialfis
Copy link
Member Author

@hcho3 If the testing framework for fast_hist created by this PR is suitable, you can add your tests based on this PR, otherwise you might duplicate the work.

@hcho3
Copy link
Collaborator

hcho3 commented Oct 29, 2018

@trivialfis Okay, I will review this PR as it stands, so that we have a proper testing infrastructure in place. We can merge this PR after 0.81 release, which is set to happen on November 1, 2018.

@trivialfis
Copy link
Member Author

@hcho3 Thanks!

hcho3
hcho3 reviewed Nov 6, 2018
View reviewed changes
src/logging.cc Show resolved Hide resolved
hcho3
hcho3 approved these changes Nov 6, 2018
View reviewed changes
Copy link
Collaborator

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

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

Overall architecture looks good to me. After this is merged, I will add more unit tests for FastHistMaker after this is merged.

src/tree/param.h Show resolved Hide resolved
tests/cpp/tree/test_quantile_hist.cc Show resolved Hide resolved
tests/cpp/tree/test_quantile_hist.cc Outdated Show resolved Hide resolved
tests/cpp/tree/test_quantile_hist.cc Outdated Show resolved Hide resolved
tests/cpp/tree/test_quantile_hist.cc

RealImpl::InitNewNode(0, gmat, row_gpairs, *(*dmat), tree);
// Manipulate the root_gain so that I don't have to invent an actual
// split. Yes, I'm cheating.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not "cheating." To me it looks like a legitimate form of testing.

src/tree/updater_quantile_hist.h Outdated Show resolved Hide resolved
src/tree/updater_quantile_hist.cc Outdated Show resolved Hide resolved
src/tree/updater_quantile_hist.h Outdated Show resolved Hide resolved
src/tree/updater_quantile_hist.cc Show resolved Hide resolved
@trivialfis
Refactor fast-hist, add tests for some updaters.
5bf2fc4 
Add unittest for prune.

Add unittest for refresh.

Refactor fast_hist.

* Remove fast_hist_param.
* Rename to quantile_hist.

Add unittests for QuantileHist.

* Refactor QuantileHist into .h and .cc file.
* Remove sync.h.
* Remove MGPU_mock test.

Rename fast hist method to quantile hist.

Address split index.
@trivialfis trivialfis merged commit 19ee0a3 into dmlc:master Nov 7, 2018
@trivialfis trivialfis deleted the unittests/updaters branch November 7, 2018 08:48
@hcho3
Copy link
Collaborator

hcho3 commented Nov 7, 2018

I will now go ahead and start working on adding unit tests for 'hist'. Thanks!

@trivialfis trivialfis mentioned this pull request Dec 11, 2018
Open
@lock lock bot locked as resolved and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hcho3 hcho3 hcho3 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.

None yet
3 participants
@trivialfis @codecov-io @hcho3