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.

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

Make `HistCutMatrix::Init' be aware of groups. #4115

Merged
merged 2 commits into from
Feb 15, 2019
Merged

Make `HistCutMatrix::Init' be aware of groups. #4115

merged 2 commits into from
Feb 15, 2019

Conversation

trivialfis
Copy link
Member

  • Add checks for group size.

Address segfault in #4098, error in quantile is not resolved.

@trivialfis trivialfis requested a review from hcho3 February 8, 2019 13:11
@hcho3
Copy link
Collaborator

hcho3 commented Feb 8, 2019

@trivialfis Yes, to clarify, weights in ranking objects will have to be per-group basis. Does this PR make this assumption? For the quantile sketch, we can replicate the group weight to cover all data points in each group.

@trivialfis
Copy link
Member Author

@hcho3 This PR accepts both per-group and per-instance until the last commit see failed java tests. I don't know how long does it take to get Travis running.

There was a test in jvm-package scala that inputs different number of weights and groups:

https://travis-ci.org/dmlc/xgboost/jobs/490537869#L1636

Please ignore the incorrect check, it's fixed in later commits. But the showed error message seems to support per-instance weighting.

@trivialfis
Copy link
Member Author

@hcho3 Ha, there it's:
https://travis-ci.org/dmlc/xgboost/jobs/490661939#L1705

So it's a buggy test and I should remove/fix it?

@hcho3
Copy link
Collaborator

hcho3 commented Feb 8, 2019

@trivialfis Yes, and we'll need to be clear about the use of per-group weights. It really doesn't make sense to give different weights to data rows in the same query session, since only the relative ordering of the data rows are important (hence learning-to-rank).

@trivialfis
Copy link
Member Author

@hcho3 Thanks for the explanation. That goes in my reading list.

@trivialfis trivialfis mentioned this pull request Feb 8, 2019
Closed
@hcho3
Copy link
Collaborator

hcho3 commented Feb 8, 2019

@trivialfis A good place to start is to learn what the ranking metric (e.g. NDCG) does. This reading helped me: http://dalelane.co.uk/blog/?p=3403

@trivialfis
Copy link
Member Author

@hcho3 Thanks for the link! That's useful.

@trivialfis trivialfis force-pushed the fix/hist-weight branch 2 times, most recently from b2d766a to 40c086f Compare February 9, 2019 21:45
@trivialfis
Copy link
Member Author

@hcho3 This should be ready now if the tests don't go nuts. Currently I can't write the documentation for ranking #4039 .

@hcho3 hcho3 mentioned this pull request Feb 10, 2019
Merged
@hcho3
Copy link
Collaborator

hcho3 commented Feb 10, 2019

@trivialfis Okay, I'll review.

hcho3
hcho3 requested changes Feb 11, 2019
View reviewed changes
src/common/hist_util.cc Outdated Show resolved Hide resolved
@hcho3
Copy link
Collaborator

hcho3 commented Feb 11, 2019

@trivialfis Can you look at my latest revision and see if it's good?

@hcho3
Copy link
Collaborator

hcho3 commented Feb 11, 2019

Oops, I think it's wrong. Let me try again

@trivialfis
Copy link
Member Author

@hcho3 Do we need to calculate different group_ind for each thread since they might be processing data belonging to different groups? And, it might be worthy to use some sort of binary search to find the index instead of a linear search.

@trivialfis
Copy link
Member Author

I can try it again tomorrow unless you beat me to it. :)

@hcho3
Copy link
Collaborator

hcho3 commented Feb 11, 2019

@trivialfis Notice that parallelization happens over columns, not rows

@trivialfis
Copy link
Member Author

@hcho3 I see. Sorry for the mistake.

@hcho3
Copy link
Collaborator

hcho3 commented Feb 11, 2019

@trivialfis No problem. I should probably add another test case with multiple batches.

@trivialfis
Copy link
Member Author

@hcho3 That would be great. I wanted to add some tests for histogram, but it's quite weird since quantile is not well tested. I don't know how to derive the solution for tests.

trivialfis
trivialfis commented Feb 11, 2019
View reviewed changes
Copy link
Member Author

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

I can change these tomorrow, but the might not be able to add the desired tests myself.

src/common/hist_util.cc Show resolved Hide resolved
src/common/hist_util.cc Outdated Show resolved Hide resolved
@hcho3
Copy link
Collaborator

hcho3 commented Feb 11, 2019

@trivialfis It turns out that currently you cannot enable external memory for fast quantile updater. See #4093. I've added a test anyway, so that we can later add external memory support to fast quantile.

@hcho3
Copy link
Collaborator

hcho3 commented Feb 12, 2019

@trivialfis Looks like I uncovered a bug in external memory. I'll remove the failing test and file an new issue.

@trivialfis
Copy link
Member Author

@hcho3 It bugs me to add such an overhead.

In the context of using external memory, does the data come in as original order? If so we can avoid adding such search completely.

@trivialfis
Copy link
Member Author

@hcho3 I will keep looking at the external memory part later. But if you know the answer then please let me know. The data loading logic is quite confusing to me. Many thanks!

@trivialfis
Copy link
Member Author

@hcho3 I added the monitor for profiling anyway. I don't think the data will come in as original order. And since the search performs in O(log n) with log(2^32) == 32, I'm gonna ignore it for now. Sorry for the noise and ready for another review. :)

@hcho3
Copy link
Collaborator

hcho3 commented Feb 13, 2019

@trivialfis For now, it is safe to assume that the external pages are out-of-order. We'll have to dive deeper into external memory when we attempt to fix #4130.

@trivialfis
Copy link
Member Author

@hcho3 Could you help taking another look? I don't think this PR has anything to do with failed R test.

trivialfis and others added 2 commits February 16, 2019 01:15
@trivialfis @hcho3
Make `HistCutMatrix::Init' be aware of groups.
5b63ed9 
* Add checks for group size.
* Simple docs.
* Revise logic for locating group ID of first row of each batch

Co-authored-by: Jiaming Yuan <jm.yuan@outlook.com>
Co-authored-by: Philip Hyunsu Cho <chohyu01@cs.washington.edu>
@trivialfis
Test `SearchGroupIndFromBaseRow'.
a181918
@trivialfis
Copy link
Member Author

@hcho3 Mac tests in Travis is not looking good. Is there anything we can do about it?

@hcho3
Copy link
Collaborator

hcho3 commented Feb 15, 2019

@trivialfis This time it’s a bug in Travis CI. Will file an issue today.

@hcho3
Copy link
Collaborator

hcho3 commented Feb 15, 2019

@trivialfis Eventually, I hope we find a better alternative for Mac builds, but unfortunately, we cannot get Mac workers with Jenkins since Amazon EC2 doesn't support it.

@hcho3
Copy link
Collaborator

hcho3 commented Feb 15, 2019

Filed a support ticket just now. Hope they get back to me soon.

@hcho3
Copy link
Collaborator

hcho3 commented Feb 15, 2019

@trivialfis Wow, they got back to me and fixed the issue right away. Builds are working now.

@trivialfis
Copy link
Member Author

@hcho3 That's really nice!

@trivialfis trivialfis merged commit 754fe81 into dmlc:master Feb 15, 2019
@trivialfis trivialfis deleted the fix/hist-weight branch February 15, 2019 20:39
This was referenced Mar 4, 2019
Merged
Merged
@lock lock bot locked as resolved and limited conversation to collaborators May 16, 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.
2 participants
@trivialfis @hcho3